diff --git a/.changeset/count-opt-out-and-permission-set-memo.md b/.changeset/count-opt-out-and-permission-set-memo.md new file mode 100644 index 0000000000..fd7b13e7eb --- /dev/null +++ b/.changeset/count-opt-out-and-permission-set-memo.md @@ -0,0 +1,57 @@ +--- +"@objectstack/metadata-protocol": minor +"@objectstack/plugin-security": patch +--- + +Stop issuing two DB queries for questions already answered earlier in the same +request (#10757). One authenticated `GET /data/:object?$top=1` measured **24 DB +queries before, 23 after** — **22** when the caller opts out of the count. +Measured with `X-OS-Debug-Timing: json` on `pnpm dev:crm`, whose `Server-Timing` +carries `db;dur=…;desc="N queries"`. + +**`$count=false` now skips the COUNT query** (`@objectstack/metadata-protocol`). +The parameter has been declared (`ODataQuerySchema.$count`), aliased on the wire +(`$count` → `count`), reserved out of the implicit-field-filter bucket, +arity-checked and boolean-coerced for a long time — and then deleted unread, so +every paginated list ran `engine.count()` whether or not the caller wanted a +total. It is honoured now: + +``` +GET /data/task?$top=25 → { records, total, hasMore } (unchanged) +GET /data/task?$top=25&$count=true → { records, total, hasMore } (unchanged) +GET /data/task?$top=25&$count=false → { records, hasMore } (no COUNT query) +``` + +Read the shape of that carefully before adopting it: + +- **Only an explicit `false` opts out.** An ABSENT `$count` still counts and + still reports `total`. OData reads absent as "omit the count", and taking that + reading here would silently strip `total` from every existing caller — none of + them send the parameter, all of them read the number. The asymmetry is + deliberate and pinned by tests. +- **`total` is OMITTED, never estimated.** `FindDataResponse.total` is declared + optional ("if requested"), so absent is the declared shape for "not + requested". A caller that opted out and then reads `total` gets `undefined`, + not a plausible-looking guess — guard the read (`total ?? undefined`) or do + not send `$count=false`. +- **`hasMore` is still answered**, from the page alone: a full page means there + may be more. Same page-local rule the `$search` path already uses. + +**A find and its COUNT resolve permission sets once, not twice** +(`@objectstack/plugin-security`). `findData` answers a paginated list with two +engine operations, and the security middleware runs on both; each pass re-read +`sys_permission_set` for the same context with identical bindings. The +resolution is now memoized per execution context — a `WeakMap` keyed on the +context object, which is built once per request and collected with it, so +nothing outlives the caller it was resolved for — and **retired by any write**: +a process-wide epoch is bumped on every `insert`/`update`/`delete` the engine +middleware sees, ahead of the `isSystem` bypass so a seeder, a package publish +or an auto-org-admin grant invalidates too. A context whose grants are rewritten +in place re-resolves as well (the memo key covers `positions`, `permissions`, +`principalKind` and the presence of `userId`). No authorization answer is reused +across a write, across a context, or across a request. + +Not a fix for the whole cost: the remaining ~22 queries per authenticated +request are session resolution, grant resolution, localization and metadata +reads that repeat on every request. Removing those needs cross-request caching +with an invalidation design, which is deliberately not in this change. diff --git a/packages/metadata-protocol/src/protocol.count-opt-out.test.ts b/packages/metadata-protocol/src/protocol.count-opt-out.test.ts new file mode 100644 index 0000000000..bab371aa55 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.count-opt-out.test.ts @@ -0,0 +1,168 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #10757 — `$count=false` skips the COUNT query. + * + * ## What was wrong + * + * `$count` has been a fully-plumbed parameter for a long time: declared in the + * spec (`ODataQuerySchema.$count`, `packages/spec/src/api/odata.zod.ts`), + * aliased on the wire (`$count` → `count`, `WIRE_DOLLAR_ALIASES`), reserved out + * of the implicit-field-filter bucket (`RESERVED_LIST_QUERY_PARAMS`), + * arity-checked (`protocol.query-param-arity.test.ts`) and boolean-coerced — + * and then DELETED unread by the protocol-key strip in `findData`. So every + * paginated list issued `engine.count()` whether or not the caller wanted a + * `total`, which on a remote database is a whole round trip per request. The + * measured trace on a real stack put it at query 24 of 24 for one + * `GET /data/:object?$top=1`. + * + * ## The two directions this suite pins, and why both are needed + * + * 1. **The OPT-OUT works** — an explicit `false` (either spelling) means no + * `engine.count()` call and no `total` key. `expect(count).not.toHaveBeenCalled()` + * is the load-bearing assertion; asserting only the absent `total` would + * stay green if a future edit ran the query and merely dropped the number, + * which is the whole cost with none of the saving. + * + * 2. **Nothing else changed** — absent `$count`, and explicit `$count=true`, + * both still count and still report `total`. This is the direction that + * makes the opt-out safe to ship: OData reads an ABSENT `$count` as "omit + * the count", and taking that reading here would silently strip `total` + * from every existing caller (none of them send the parameter, all of them + * read the number). The asymmetry is deliberate, so it is pinned rather + * than left to be "tidied up" later. + * + * `total` is OMITTED rather than estimated — `FindDataResponseSchema` declares + * it optional ("if requested"), and a page-local guess handed back to a caller + * who declined the real number is how an estimate ends up rendered as a record + * count. `hasMore` is still answered from the page alone. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { + name: 'invoice', + nameField: 'name', + fields: { + name: { name: 'name', type: 'text' }, + status: { name: 'status', type: 'text' }, + }, +}; + +function makeProtocol(pageSize: number) { + const find = vi.fn(async () => Array.from({ length: pageSize }, (_, i) => ({ id: `r${i}` }))); + const count = vi.fn(async () => 3125); + const engine = { + registry: { getObject: (n: string) => (n === 'invoice' ? SCHEMA : undefined) }, + find, + count, + aggregate: vi.fn(async () => [] as unknown[]), + }; + return { p: new ObjectStackProtocolImplementation(engine as any), find, count }; +} + +describe('[#10757] findData honours $count=false', () => { + describe('opt-out — the COUNT query is not issued', () => { + // Both wire spellings reach the same normalized `count` slot; a fix that + // read only one of them would leave the other paying for the query. + for (const spelling of ['$count', 'count'] as const) { + it(`?${spelling}=false skips engine.count() and omits total`, async () => { + const { p, count } = makeProtocol(1); + + const result = await p.findData({ + object: 'invoice', + query: { $top: 1, [spelling]: 'false' }, + } as never); + + expect(count).not.toHaveBeenCalled(); + expect('total' in (result as object)).toBe(false); + }); + } + + it('accepts the already-boolean form a POST body carries', async () => { + const { p, count } = makeProtocol(1); + + const result = await p.findData({ + object: 'invoice', + query: { $top: 1, count: false }, + } as never); + + expect(count).not.toHaveBeenCalled(); + expect('total' in (result as object)).toBe(false); + }); + + it('still answers hasMore from the page: a FULL page means there may be more', async () => { + const { p } = makeProtocol(10); + + const result = await p.findData({ + object: 'invoice', + query: { $top: 10, $count: 'false' }, + } as never); + + expect(result.hasMore).toBe(true); + }); + + it('…and a SHORT page means there are not', async () => { + const { p } = makeProtocol(3); + + const result = await p.findData({ + object: 'invoice', + query: { $top: 10, $count: 'false' }, + } as never); + + expect(result.hasMore).toBe(false); + }); + + it('leaves `count` off the engine option bag (it is a protocol-layer flag)', async () => { + const { p, find } = makeProtocol(1); + + await p.findData({ object: 'invoice', query: { $top: 1, $count: 'false' } } as never); + + const bag = (find.mock.calls[0] as unknown[])[1] as Record; + expect('count' in bag).toBe(false); + expect('$count' in bag).toBe(false); + }); + }); + + describe('unchanged for every caller that does not opt out', () => { + it('an ABSENT $count still counts and still reports total', async () => { + const { p, count } = makeProtocol(1); + + const result = await p.findData({ object: 'invoice', query: { $top: 1 } } as never); + + expect(count).toHaveBeenCalledTimes(1); + expect(result.total).toBe(3125); + expect(result.hasMore).toBe(true); + }); + + it('an explicit $count=true still counts and still reports total', async () => { + const { p, count } = makeProtocol(1); + + const result = await p.findData({ + object: 'invoice', + query: { $top: 1, $count: 'true' }, + } as never); + + expect(count).toHaveBeenCalledTimes(1); + expect(result.total).toBe(3125); + }); + + it('$count=false without a limit is a no-op — the full set is already the total', async () => { + // No `limit` ⇒ the whole result set came back, so `records.length` IS + // the total and `engine.count()` was never called even before #10757. + // Pinned so the opt-out cannot accidentally start suppressing a total + // that costs nothing. + const { p, count } = makeProtocol(4); + + const result = await p.findData({ + object: 'invoice', + query: { $count: 'false' }, + } as never); + + expect(count).not.toHaveBeenCalled(); + expect(result.total).toBe(4); + expect(result.hasMore).toBe(false); + }); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 8869b9c8e7..3e1d400d4c 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -8881,7 +8881,8 @@ export class ObjectStackProtocolImplementation implements // ride the spread into the engine AST and OVERRIDE the resolved // object, splitting `ast.object` from the table actually queried — // a mismatch is refused, never resolved by picking a winner. - // - `count`: a response-shape flag this method consumed above. + // - `count`: a response-shape flag this method consumed above (and, + // since #10757, one it actually HONOURS — see `countOptOut` below). // - QueryAST tombstones (`cursor`/`joins`/`windowFunctions`/ // `distinct`): reserved at the wire gate so they are not read as // field filters; on the wire they stay ignored-with-tombstone-docs @@ -8899,6 +8900,22 @@ export class ObjectStackProtocolImplementation implements err.code = 'QUERY_OBJECT_MISMATCH'; throw err; } + // [#10757] `$count=false` — read the flag BEFORE the strip below deletes + // it, because the strip is what kept it from ever being honoured: the + // parameter has been declared (`ODataQuerySchema.$count`, + // `packages/spec/src/api/odata.zod.ts`), aliased (`$count` → `count`, + // {@link WIRE_DOLLAR_ALIASES}), reserved from the implicit-field-filter + // bucket ({@link RESERVED_LIST_QUERY_PARAMS}), arity-checked and boolean- + // coerced — and then deleted unread, so every list request paid for the + // COUNT query below whether or not the caller wanted a `total`. + // + // Only an EXPLICIT `false` opts out. An absent `$count` keeps today's + // behaviour (count runs, `total` is reported) rather than taking OData's + // "absent means omit" reading: every existing caller sends nothing and + // reads `total`, so the OData default would silently break all of them. + // The parameter is therefore an opt-OUT here, and that asymmetry is + // deliberate — see the changeset for the wording that ships to consumers. + const countOptOut = options.count === false; for (const k of ['object', 'count', 'joins', 'windowFunctions', 'cursor', 'distinct', 'having']) { delete options[k]; } @@ -8915,7 +8932,7 @@ export class ObjectStackProtocolImplementation implements // reporting a wrong total. const pageLimit = typeof options.limit === 'number' && options.limit > 0 ? options.limit : undefined; const pageOffset = typeof options.offset === 'number' && options.offset > 0 ? options.offset : 0; - let total = records.length; + let total: number | undefined = records.length; let hasMore = false; if (pageLimit !== undefined) { // `distinct` used to suppress the count here too — #4286 finding 2: @@ -8923,18 +8940,47 @@ export class ObjectStackProtocolImplementation implements // that never deduplicated a row. Removed with `query.distinct` // (tombstoned in spec 17); `total`/`hasMore` are truthful again. const countable = options.search == null; - if (countable) { + if (countOptOut) { + // [#10757] The caller said it does not need `total`, so the + // COUNT query is not issued at all — that is the whole point of + // the parameter, and on a remote database it is a full round + // trip saved per list request. + // + // `total` is OMITTED rather than estimated. `FindDataResponse` + // declares it optional ("Total number of records matching the + // filter (IF REQUESTED)", + // `packages/spec/src/api/protocol.zod.ts`), so absent is the + // declared shape for "not requested" — and it is the only + // honest one: the `search` branch below reports an estimate + // because it has no better number to give, while here a real + // number was available and the caller declined it. Handing back + // a plausible-looking guess under those circumstances is how a + // page-local estimate ends up rendered as a record count. + // + // `hasMore` is still answered, from the page alone: a FULL page + // means there may be more. Same page-local rule the search + // branch uses, and it never over-reports the data — it can only + // say "maybe more" on an exactly-full last page. + hasMore = records.length === pageLimit; + total = undefined; + } else if (countable) { + // [#10757] `counted` is a separate, always-assigned local so + // `hasMore` below compares against a `number`: `total` became + // optional when `$count=false` gained the right to omit it, and + // TypeScript cannot narrow it back across the try/catch. + let counted: number; try { - total = await this.engine.count(request.object, { + counted = await this.engine.count(request.object, { where: options.where, context: options.context, } as any); } catch { // engine.count() has its own find().length fallback; if it still // throws, degrade to a page-local total rather than failing the list. - total = pageOffset + records.length; + counted = pageOffset + records.length; } - hasMore = pageOffset + records.length < total; + total = counted; + hasMore = pageOffset + records.length < counted; } else { hasMore = records.length === pageLimit; total = pageOffset + records.length + (hasMore ? 1 : 0); @@ -8943,7 +8989,11 @@ export class ObjectStackProtocolImplementation implements return { object: request.object, records, - total, + // [#10757] Omitted, not `undefined`-valued: a JSON body carrying + // `"total": null` (or a key some serializers keep) reads as "the + // total is nothing", which is a different claim from "no total was + // requested". The key is simply absent. + ...(total === undefined ? {} : { total }), hasMore, }; } diff --git a/packages/plugins/plugin-security/src/permission-set-resolution-memo.test.ts b/packages/plugins/plugin-security/src/permission-set-resolution-memo.test.ts new file mode 100644 index 0000000000..35c0a977ee --- /dev/null +++ b/packages/plugins/plugin-security/src/permission-set-resolution-memo.test.ts @@ -0,0 +1,229 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10757] The engine middleware resolves a context's permission sets ONCE, and + * a WRITE retires that resolution. + * + * ## The duplicate + * + * `findData` answers a paginated list with two engine operations — `find` for + * the page, `count` for the total — and this middleware runs on both. Each pass + * re-resolved the same permission sets for the same context, so one + * `GET /data/:object?$top=1` sent `select * from sys_permission_set where name + * in (…)` twice, back to back, identical bindings. Measured on a real stack + * (`pnpm dev:crm`, `X-OS-Debug-Timing: json`): queries 21 and 23 of 24 before, + * 23 queries after. + * + * ## Why this suite is mostly about INVALIDATION + * + * Reusing an authorization answer is only safe while the question cannot have + * changed, so the assertions that matter here are the ones that prove it is + * NOT reused when it could have. Three independent ways the answer can change, + * one block each: + * + * - **a write** — a permission change lands as a write to `sys_permission_set` + * / `sys_user_permission_set` / `sys_position_*` through this very engine. + * The epoch bump sits ahead of the `isSystem` bypass on purpose: the writes + * most likely to change what a caller may see are system ones (the seeder, a + * package publish, the auto-org-admin grant), and a guard that only saw user + * writes would leave a memo standing across exactly those. + * - **a different principal** — a second context object never reads the + * first's answer, whatever it holds. + * - **the same object, rewritten in place** — grants edited on a live context + * change the memo key, so the resolution is redone. + * + * The DEDUPE assertion (`toHaveBeenCalledTimes(1)`) is the only one that would + * pass on `origin/main`; every invalidation assertion below is green in both + * directions and is a GUARD, not evidence. That is the intended split: the + * saving is one measurement, and the safety is a fence around it. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { PermissionSet } from '@objectstack/spec/security'; + +import { SecurityPlugin } from './security-plugin.js'; + +/** Resolvable from metadata, so only `custom_role` below reaches the db loader. */ +const baselineSet: PermissionSet = { + name: 'member_default', + label: 'Member', + objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } }, +} as never; + +/** + * The db-loader harness. `find` is the SOLE observable: every call it records is + * one `sys_permission_set` round trip the deployment would really have made. + */ +const makeHarness = () => { + const fields: Record = {}; + for (const f of ['id', 'organization_id', 'owner_id', 'name']) fields[f] = { name: f }; + const baseSchema = { name: 'task', fields }; + let middleware: ((opCtx: unknown, next: () => Promise) => Promise) | undefined; + // The options bag is declared even though the fake ignores it: the assertion + // below reads `call[1]`, and a one-parameter mock types its call tuple as + // length 1 — `tsc` refuses the index, and this package's test layer is + // ratcheted (`pnpm check:type-check-debt`). + const find = vi.fn(async (object: string, _options?: unknown) => + object === 'sys_permission_set' + ? [{ id: 'ps-1', name: 'custom_role', label: 'Custom', object_permissions: '{}' }] + : [], + ); + const ql = { + registerMiddleware: (mw: never) => { + if (!middleware) middleware = mw; + }, + getSchema: () => baseSchema, + find, + findOne: vi.fn(async () => null), + }; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: ql, + metadata: { get: async () => baseSchema, list: async () => [baselineSet] }, + }; + const ctx = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + registerService: vi.fn(), + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + return { + ctx, + find, + /** + * How many reads THE DB LOADER has issued so far. + * + * Matched on the loader's own predicate shape (`name: { $in: [...] }`) + * rather than on the object name: several unrelated probes read + * `sys_permission_set` by single name on this path (the ADR-0095 + * auto-org-admin grant, the ADR-0090 audience-binding suggestions), and + * counting those would make the number mean "reads of a table" instead of + * "resolutions of this context" — the quantity under test. + */ + permissionSetReads: () => + find.mock.calls.filter((c) => { + if (c[0] !== 'sys_permission_set') return false; + const where = (c[1] as { where?: { name?: unknown } } | undefined)?.where; + return Array.isArray((where?.name as { $in?: unknown[] } | undefined)?.$in); + }).length, + run: async (opCtx: unknown) => { + if (!middleware) throw new Error('middleware never registered'); + await middleware(opCtx, async () => {}); + return opCtx; + }, + }; +}; + +const boot = async () => { + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeHarness(); + await plugin.init(harness.ctx as never); + await plugin.start(harness.ctx as never); + return harness; +}; + +/** A live execution context — one object, threaded through a request's ops. */ +const requestContext = () => ({ + userId: 'u1', + tenantId: 'org-1', + positions: ['custom_role'], + permissions: [] as string[], +}); + +const readOp = (context: unknown, operation: 'find' | 'count') => ({ + object: 'task', + operation, + ast: { object: 'task', where: undefined }, + options: { where: undefined }, + context, +}); + +describe('[#10757] permission-set resolution is memoized per execution context', () => { + it('resolves ONCE for the find/count pair of a single request', async () => { + const harness = await boot(); + const context = requestContext(); + + await harness.run(readOp(context, 'find')); + await harness.run(readOp(context, 'count')); + + expect(harness.permissionSetReads()).toBe(1); + }); + + describe('…and re-resolves whenever the answer could have changed', () => { + it('after a WRITE — even a system write, which bypasses every other gate', async () => { + const harness = await boot(); + const context = requestContext(); + + await harness.run(readOp(context, 'find')); + // The one shape the guard has to catch and the `isSystem` bypass would + // otherwise hide: a seeder / package publish / auto-grant rewriting the + // RBAC tables mid-flight. + await harness.run({ + object: 'sys_user_permission_set', + operation: 'insert', + data: { user_id: 'u1', permission_set_id: 'ps-9' }, + context: { isSystem: true }, + }); + await harness.run(readOp(context, 'count')); + + expect(harness.permissionSetReads()).toBe(2); + }); + + it('after a write by ANOTHER context — the epoch is process-wide, not per caller', async () => { + const harness = await boot(); + const context = requestContext(); + + await harness.run(readOp(context, 'find')); + await harness.run({ + object: 'sys_permission_set', + operation: 'update', + id: 'ps-1', + data: { label: 'Renamed' }, + context: { isSystem: true }, + }); + await harness.run(readOp(context, 'count')); + + expect(harness.permissionSetReads()).toBe(2); + }); + + it('for a DIFFERENT context object holding identical grants', async () => { + const harness = await boot(); + + // Two requests by the same user resolve independently: the memo is keyed + // on the context OBJECT, so nothing survives the request that built it. + await harness.run(readOp(requestContext(), 'find')); + await harness.run(readOp(requestContext(), 'find')); + + expect(harness.permissionSetReads()).toBe(2); + }); + + it('when the SAME context object has its grants rewritten in place', async () => { + const harness = await boot(); + const context = requestContext(); + + await harness.run(readOp(context, 'find')); + context.positions = ['some_other_role']; + await harness.run(readOp(context, 'find')); + + expect(harness.permissionSetReads()).toBe(2); + }); + }); + + it('hands every caller its own array — the memo is not an aliasing channel', async () => { + const harness = await boot(); + const context = requestContext(); + const opA: Record = readOp(context, 'find'); + const opB: Record = readOp(context, 'count'); + + await harness.run(opA); + await harness.run(opB); + + // Both passes injected a read scope built from the SAME resolution, and + // neither pass could have mutated the other's list on the way. + expect(harness.permissionSetReads()).toBe(1); + expect(opA.ast.object).toBe('task'); + expect(opB.ast.object).toBe('task'); + }); +}); diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 4478e1917b..356accf4f3 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -721,6 +721,58 @@ export class SecurityPlugin implements Plugin { */ private readonly objectSecurityMetaCache = new Map(); private dbLoader?: (names: string[]) => Promise; + /** + * [#10757] Per-EXECUTION-CONTEXT memo for + * {@link SecurityPlugin.resolvePermissionSetsForContext}. + * + * ## The duplicate it removes + * + * `findData` (`@objectstack/metadata-protocol`) answers a paginated list with + * TWO engine operations — `find` for the page, then `count` for the total — + * and this middleware runs on both. Each pass re-resolved the same permission + * sets for the same context, so one `GET /data/:object?$top=1` issued + * `select * from sys_permission_set where name in (...)` TWICE, back to back, + * with identical bindings (measured on a real stack: queries 21 and 23 of 24). + * On a database a network hop away that is a full round trip spent asking a + * question already answered microseconds earlier. + * + * ## Why keying on the context object is the request scope + * + * The execution context is built ONCE per request and threaded unchanged + * through every operation of that request — `@objectstack/rest` memoizes it + * per `req` (`RestServer.execCtxMemo`), and the runtime/MCP dispatchers each + * assemble one per dispatch. Keying a `WeakMap` on that object therefore + * scopes the memo to exactly the resolution the context represents, and the + * entry is collected with it: no TTL, no module-level table keyed by user id, + * nothing that outlives the caller it was resolved for. + * + * ## Why a write epoch, and not object identity alone + * + * Object identity alone would be a staleness window rather than a + * de-duplication: a context that performs a WRITE and then reads again is + * entitled to see the write. A permission change lands as a write to + * `sys_permission_set` / `sys_user_permission_set` / `sys_position_*` through + * this very engine, so {@link SecurityPlugin.writeEpoch} is bumped on every + * write operation the middleware sees -- before the `isSystem` bypass, so a + * seeder, a package publish or the auto-org-admin grant invalidates too -- and + * an entry is reused only while the epoch it was resolved at still stands. + * Any write, by ANY context in this process, retires every entry. + * + * What that leaves is exactly one thing: two reads by one context with no + * intervening write, which is the find/count pair above and is the definition + * of a duplicate question. It is NOT a cache -- nothing here survives a write, + * and nothing here survives its context. + */ + private readonly permissionSetMemo = new WeakMap< + object, + { epoch: number; key: string; sets: Promise } + >(); + /** + * [#10757] Monotonic counter bumped on every engine WRITE this middleware + * sees. Read only by {@link SecurityPlugin.permissionSetMemo}; see its doc for + * why the guard exists and what it is guarding against. + */ + private writeEpoch = 0; private logger: { info?: (...a: any[]) => void; warn?: (...a: any[]) => void; error?: (...a: any[]) => void } = {}; constructor(options: SecurityPluginOptions = {}) { @@ -853,6 +905,12 @@ export class SecurityPlugin implements Plugin { this.tenancyDisabledCache.clear(); this.cbpRelCache.clear(); this.objectSecurityMetaCache.clear(); + // [#10757] A permission set can be DECLARED in metadata, so a metadata + // change is a permission change even when no row was written. Retiring + // the per-context memo here keeps it on the same invalidation footing + // as every other metadata-derived cache above, rather than being the + // one that survives a Studio edit. + this.writeEpoch++; }); } @@ -1184,6 +1242,17 @@ export class SecurityPlugin implements Plugin { // Register security middleware ql.registerMiddleware(async (opCtx: any, next: () => Promise) => { + // [#10757] Retire every memoized permission-set resolution the moment a + // WRITE passes through the engine. Deliberately the FIRST statement in + // the middleware — ahead of the `isSystem` bypass immediately below — + // because the writes most likely to change what a caller may see are + // system ones: the platform seeder, a package publish, the auto-org-admin + // grant. A guard that only saw user writes would leave a memo standing + // across exactly the grants that matter. See {@link permissionSetMemo}. + if (opCtx.operation === 'insert' || opCtx.operation === 'update' || opCtx.operation === 'delete') { + this.writeEpoch++; + } + // System operations bypass security if (opCtx.context?.isSystem) { return next(); @@ -3884,6 +3953,31 @@ export class SecurityPlugin implements Plugin { return true; } + /** + * [#10757] The memo key: every field of the execution context this resolution + * reads. Two contexts that agree on all of them get the same answer, and a + * context whose grants were rewritten in place (rather than replaced) gets a + * different key and is re-resolved. + * + * Spelled out rather than derived from the object, deliberately: a + * `JSON.stringify(context)` would fold in fields the resolution never reads + * (making the memo miss for no reason) and would silently start keying on any + * field added later — including one that should NOT participate. The list is + * short because the inputs are: {@link resolvePermissionSetsForContext} reads + * `positions`, `permissions`, `principalKind` and the PRESENCE of `userId`, + * and nothing else off the context. + */ + private permissionSetMemoKey(context: any): string { + const positions = Array.isArray(context?.positions) ? context.positions : []; + const permissions = Array.isArray(context?.permissions) ? context.permissions : []; + return JSON.stringify([ + positions, + permissions, + context?.principalKind ?? null, + context?.userId ? 1 : 0, + ]); + } + /** * Resolve the effective permission sets for an execution context — positions + * explicit permission sets, with the configured baseline applied both as an @@ -3891,9 +3985,47 @@ export class SecurityPlugin implements Plugin { * (when named ones resolved to nothing). Shared by the engine middleware and * {@link getReadFilter} so both enforce identical RLS. May throw if the * underlying metadata/db resolution fails (callers fail-closed). + * + * [#10757] Memoized per execution context and retired by any write — see + * {@link permissionSetMemo} for the duplicate this removes, why the context + * object IS the request scope, and why the write epoch is what makes reuse a + * de-duplication rather than a staleness window. A REJECTION is never + * retained: the entry is dropped so the next caller re-resolves (callers + * fail closed on a throw, so a memoized failure would deny for the rest of + * the context's life on a fault that may already be over). */ private async resolvePermissionSetsForContext( context: any, + ): Promise { + // A primitive/absent context has no identity to key on — and no grants to + // resolve either. Straight through. + if (!context || typeof context !== 'object') { + return this.resolvePermissionSetsForContextUnmemoized(context); + } + const key = this.permissionSetMemoKey(context); + const hit = this.permissionSetMemo.get(context); + // Every caller gets its OWN array, exactly as before this memo existed — + // a shared array would be a new aliasing hazard on top of a query saving, + // and no caller should have to know which of the two it holds. (The + // PermissionSet objects inside were already shared instances: they come + // from the metadata/bootstrap registries, not from this call.) + if (hit && hit.epoch === this.writeEpoch && hit.key === key) { + return hit.sets.then((s) => [...s]); + } + const sets = this.resolvePermissionSetsForContextUnmemoized(context); + const entry = { epoch: this.writeEpoch, key, sets }; + this.permissionSetMemo.set(context, entry); + sets.catch(() => { + if (this.permissionSetMemo.get(context) === entry) { + this.permissionSetMemo.delete(context); + } + }); + return sets.then((s) => [...s]); + } + + /** The actual resolution — see {@link resolvePermissionSetsForContext}. */ + private async resolvePermissionSetsForContextUnmemoized( + context: any, ): Promise { const positions = context?.positions ?? []; const explicitPermissionSets = context?.permissions ?? [];