diff --git a/.changeset/29362-role-holders-complete.md b/.changeset/29362-role-holders-complete.md new file mode 100644 index 00000000000..fd27b408d7b --- /dev/null +++ b/.changeset/29362-role-holders-complete.md @@ -0,0 +1,25 @@ +--- +'@objectstack/plugin-sharing': patch +'@objectstack/plugin-approvals': patch +--- + +fix(plugins)!: a role or membership tier held by more than 10,000 members reaches every holder in sharing and approvals, and a membership read that fails is refused loudly instead of answered with an empty set + +Clause-②: no (narrowing) +Metadata-protocol: none + + + +**BREAKING** only where a `sys_member` read fails: the role expansion in sharing and the membership-tier expansion in approvals used to answer an empty list then, and now refuse. + +Both expansions read `sys_member` once with a 10,000-row bound and took that read as the whole holder list. They also turned any read error into `[]`. + +- **Every holder, nobody else.** `TeamGraphService.expandRoleUsers` (plugin-sharing: the `sys_member.role` limb of every `position` sharing recipient, and `expandPrincipal('role')`) and the approvals membership-tier expansion (an `org_membership_level` approver, and the second limb of every `position` approver) now read `sys_member` in pages of 10,000 ordered by id until the holders are exhausted. Past 10,000 holders, a sharing rule used to grant nothing to the holders past the 10,000th row, and an approval used to route without them. Both now reach every holder. +- **Smaller roles are unchanged.** A role or tier held by fewer than 10,000 members issues one read, as before, and answers the same set. +- **Loud, never short or empty.** A read that fails, or a page the driver returned without honouring the id cursor, now throws instead of answering `[]`. + - Sharing: the expansion rejects, so that rule's reconcile or revoke pass fails, its callers log their existing per-rule warning, and the grants it already holds stay as they were. Before, the retired-recipient revoke compared a rule's materialised grants against that empty answer and revoked every one of them. + - Approvals: the approver slot falls back to its literal (`position:NAME`, which whoever holds the position can still act on, or `org_membership_level:NAME`), and the warning now reads `could not be expanded — a directory or read-filter lookup failed` and carries its `error`. Before, a `position` approver routed to its assignment-table holders alone, which looked like a complete slate, and an `org_membership_level` approver logged "expanded to nobody". +- **The per-record sharing pass now resolves per rule.** `SharingRuleService.evaluateAllForRecord` (the pass a record write runs) used to reject as a whole when any one rule threw. Now a rule whose recipients cannot be read is left out of the returned results, its existing grants stay, and it logs `[sharing-rule] record reconcile failed for rule` with the rule and the error. The record's other rules still reconcile. This matches what `evaluateAllRulesForObject` already did. +- **A missing membership object is still no holders.** A stack that never creates or never registers `sys_member` keeps answering `[]`. That is judged by the shared missing-table predicate, or by the engine's `OBJECT_NOT_FOUND` for `sys_member`, rather than by swallowing every error. + +**The remedy.** None is needed. A refusal means the membership table could not be read: fix that read, and the next expansion succeeds. A failed read is never cached. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 4e7f4578aa2..4ef71390e72 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -455,7 +455,11 @@ What an entry that resolves to nobody leaves on the slate depends on its kind: decide it (the admin-override callout under [Acting on requests in the console](#acting-on-requests-in-the-console)). Beside entries that do resolve, a literal is a slot like any other, so a - `behavior` that needs every slot (`unanimous`) waits on it too. + `behavior` that needs every slot (`unanimous`) waits on it too. A `position` or + `org_membership_level` entry whose `sys_member` read failed keeps its literal + the same way. It logs `could not be expanded — a directory or read-filter + lookup failed` with the `error`, instead of `expanded to nobody`, and never + routes to the holders that happened to read. - **A slate left empty, or holding only literals,** goes to the node's `onEmptyApprovers` policy, whose four values are listed under [Approving across organizations](#cross-org-approvers). Under the default, diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index be0dd52a6d6..857865f158e 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -21,7 +21,7 @@ import { ExpressionEngine, collectCelRootIdentifiers } from '@objectstack/formul // writer-local re-derivation here was rejected by name (Option B): it would be // a third answer to a question the codebase already answered two ways. import { createRecordOrganizationResolver, type RecordOrganizationResolver } from '@objectstack/metadata-core'; -import { keysetWalk, strandedDecisionFailure } from '@objectstack/types'; +import { isMissingTableError, keysetWalk, strandedDecisionFailure } from '@objectstack/types'; // [#15981 / #16166] Every built-in identity NAME is deliberately absent here: // both arms of `isOverrideActor` read an ADR-0095 capability rung, never a name. // `BUILTIN_IDENTITY_PLATFORM_ADMIN` went with the platform arm's name read and @@ -1118,6 +1118,31 @@ const OOO_MAX_CHAIN = 8; */ const MEMBER_SCREEN_READ_LIMIT = 50000; +/** + * [#29362] Rows per page of the membership-tier read + * ({@link ApprovalService.expandMembershipTierUsers}) — the bound the read + * always carried, so a tier held by fewer people issues the one read it always + * did, and a tier held by more is read in further pages rather than cut. + */ +const MEMBER_TIER_PAGE_ROWS = 10000; + +/** + * [#29362] Did a `sys_member` read fail because this stack has no membership + * object at all — the one failure whose empty answer is the truth? Two + * spellings of the same absence, each attributed to `sys_member` so a failure + * about another object is not evidence about this one: the driver's + * missing-table refusal (the shared `isMissingTableError`, the table + * registered but never created), and the engine's `OBJECT_NOT_FOUND` (a + * composition that never registers the object, refused before any driver is + * asked — the reading `sys-metadata-repository.ts` gives its own history + * object). Anything else is an outage and stays loud. + */ +function membershipObjectAbsent(err: unknown): boolean { + if (isMissingTableError(err, 'sys_member')) return true; + const refused = err as { code?: unknown; object?: unknown } | null | undefined; + return refused?.code === 'OBJECT_NOT_FOUND' && refused.object === 'sys_member'; +} + /** * Approver types resolved by QUERYING a graph rather than by taking `value` * literally (#3807). Each can legitimately come back empty — an unstaffed @@ -2551,6 +2576,9 @@ export class ApprovalService implements IApprovalService { // The user a `manager` approver resolved FROM, kept for the warning below // when no manager comes back. let managerSubject: unknown; + // [#29362] The directory failure the catch below took, kept so the warning + // can say the read FAILED rather than that the slot expanded to nobody. + let directoryFailure: string | undefined; try { if (type === 'team') { // #10230: the request's OWN organization, not `directoryOrg`. They are @@ -2589,7 +2617,11 @@ export class ApprovalService implements IApprovalService { if (mgr) return this.applyOooDelegation(mgr, now, organizationId, substitutions); } } - } catch { /* a directory lookup failed → handled below as resolving to nobody */ } + } catch (err) { + // A directory lookup failed → handled below as resolving to nobody, and + // the graph-type warning names the failure (#29362). + directoryFailure = err instanceof Error ? err.message : String(err); + } // #22558: a PERSON type that resolved to nobody contributes NO slot — see // PERSON_APPROVER_TYPES for why its `type:value` literal could never be // taken. With nothing else on the slate, the request reaches the node's @@ -2656,12 +2688,25 @@ export class ApprovalService implements IApprovalService { // follows. The old ending ("the request cannot advance until someone is // added") restated a fate this resolver cannot know, and was false on // the default path. - this.logger?.warn?.( - `[approvals] approver '${type}:${a.value}' expanded to nobody — the slot names no one, so no one is ` - + `asked through it. Staff it or re-point the approver; a slate left naming no one is decided by the ` - + `node's onEmptyApprovers.`, - { type, value: a.value, organizationId: organizationId ?? null }, - ); + if (directoryFailure !== undefined) { + // [#29362] A failed read is not an unstaffed approver: say which one + // this is, and carry the error, so an operator does not go staffing a + // position whose holders simply could not be read. The try above also + // wraps the cross-org read filter (`bounded`), so the line names both. + this.logger?.warn?.( + `[approvals] approver '${type}:${a.value}' could not be expanded — a directory or read-filter lookup failed, so the ` + + `slot falls back to its literal rather than route to part of its people. Fix the read (see error); ` + + `a slate left naming no one is decided by the node's onEmptyApprovers.`, + { type, value: a.value, organizationId: organizationId ?? null, error: directoryFailure }, + ); + } else { + this.logger?.warn?.( + `[approvals] approver '${type}:${a.value}' expanded to nobody — the slot names no one, so no one is ` + + `asked through it. Staff it or re-point the approver; a slate left naming no one is decided by the ` + + `node's onEmptyApprovers.`, + { type, value: a.value, organizationId: organizationId ?? null }, + ); + } } // #27397: a `user` that resolved to no one adds no slot — `user:` // is no acting address (`approver-address.ts`), and the bare value is the @@ -3355,16 +3400,54 @@ export class ApprovalService implements IApprovalService { * {@link expandPositionUsers} carries the ruling both reads inherit * (#8613 / commit 04d03c3a0) — this method is also the second limb of that union, so a * change here changes position routing too. + * + * [#29362] Read to COMPLETION, or refused — never a shortened or an invented + * empty slate. Pages of {@link MEMBER_TIER_PAGE_ROWS} through the platform's + * one keyset walk (`keysetWalk`, `@objectstack/types`), ordered by `id`, so a + * tier held past the bound routes to every holder rather than the first + * 10,000 rows. A read that fails, or a walk that cannot vouch for completion + * (a row without an `id`, a page that did not honour the cursor), THROWS: + * {@link resolveApproverSpec}'s directory catch then lets the slot fall back + * to its literal and says the read failed. Answering `[]` instead routed a + * `position` approver to its assignment holders alone — a shrunk slate that + * looks complete — and logged an `org_membership_level` one as "expanded to + * nobody", indistinguishable from a tier no one holds. A membership object + * that does not exist is the one failure that IS an answer (a stack that never + * materialises `sys_member` has no tier holders) and keeps answering `[]`; + * see {@link membershipObjectAbsent}. */ private async expandMembershipTierUsers(tier: string, organizationId?: string | null): Promise { if (!tier) return []; - const filter: any = { role: tier }; + const filter: Record = { role: tier }; if (organizationId) filter.organization_id = organizationId; - let rows: any[] = []; + const scope = `the holders of membership tier '${tier}'${organizationId ? ` in organization '${organizationId}'` : ''}`; + const users = new Set(); + const walk = keysetWalk>( + (page) => this.engine.find('sys_member', { ...page, fields: ['id', 'user_id'], context: SYSTEM_CTX }), + { where: filter, pageSize: MEMBER_TIER_PAGE_ROWS }, + ); try { - rows = await this.engine.find('sys_member', { filter, fields: ['user_id'], limit: 10000, context: SYSTEM_CTX } as any); - } catch { rows = []; } - return Array.from(new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean))); + for await (const page of walk.pages()) { + for (const row of page) { + const uid = String(row?.user_id ?? ''); + if (uid) users.add(uid); + } + } + } catch (err) { + if (membershipObjectAbsent(err)) return []; + throw Object.assign(new Error( + `[approvals] ${scope} could not be read: ${err instanceof Error ? err.message : String(err)}. ` + + 'Routing to a shortened slate would leave holders unasked, so the expansion refuses instead.', + ), { cause: err }); + } + if (walk.truncated) { + throw new Error( + `[approvals] ${scope} could not be read to completion: the read by id could not continue past a page ` + + '(a row without an id, or a page that repeated the cursor). Routing to a shortened slate would leave ' + + 'holders unasked, so the expansion refuses instead.', + ); + } + return Array.from(users); } /** diff --git a/packages/plugins/plugin-approvals/src/member-tier-holders-complete.integration.test.ts b/packages/plugins/plugin-approvals/src/member-tier-holders-complete.integration.test.ts new file mode 100644 index 00000000000..47d16e816ef --- /dev/null +++ b/packages/plugins/plugin-approvals/src/member-tier-holders-complete.integration.test.ts @@ -0,0 +1,184 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#29362] The membership-tier expansion (`sys_member.role`) answers EVERY + * holder of a tier, or refuses — never a shortened or an invented empty set. + * + * `expandMembershipTierUsers` is the whole of an `org_membership_level` + * approver and the second limb of every `position` approver (ADR-0057 D4 + * transition source). It used to read `sys_member` once with a 10,000-row + * bound and take that read as the holder list, and it turned a read that threw + * into `[]`: + * + * - past 10,000 holders the request routed to a slate missing the holders + * past the 10,000th row, with no signal; + * - a read that failed routed a `position` approver to its assignment-table + * holders ALONE — a shrunk slate that looks exactly like a complete one — + * and an `org_membership_level` approver logged "expanded to nobody", + * indistinguishable from a tier nobody holds. + * + * What a caller sees now: a failed read throws out of the expansion, the + * approver-spec resolver's directory catch takes it, and the slot falls back + * to its literal (`position:

`, acted on by whoever holds the position at + * decision time; `org_membership_level:`, decided by the override) with a + * warning that names the failed read and carries its `error` — never a slate + * cut down to the part that happened to read. + * + * Measured on a REAL engine (ObjectQL + SqlDriver over in-memory SQLite) + * carrying the shipped `SysMember`, because the completion is a keyset read + * (`id > cursor`, ordered by `id`) that a hand-written double cannot vouch for. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SysMember, SysOrganization } from '@objectstack/platform-objects/identity'; +import { ApprovalService } from './approval-service.js'; + +const ORG = 'org_large'; +const ELSEWHERE = 'org_elsewhere'; +const TIER = 'admin'; +/** The bound the tier read is paged at. */ +const PAGE = 10000; + +const pad = (n: number) => String(n).padStart(6, '0'); + +interface Rig { + engine: ObjectQL; + svc: ApprovalService; + warn: ReturnType; + /** Every holder of {@link TIER} in {@link ORG}, sorted. */ + holders: string[]; + /** The tier reads the driver ran (`… from sys_member where … role = ? …`). */ + tierReads: () => number; +} + +const engines: ObjectQL[] = []; +afterEach(async () => { + vi.restoreAllMocks(); + while (engines.length) { + try { await engines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +/** + * `n` holders of {@link TIER} in {@link ORG}, plus a holder of the same tier in + * another organization and a member of {@link ORG} in another tier — the two + * rows the predicate must keep out. Membership ids are scrambled against the + * user ids, so an unordered first read is not the lowest ids. + */ +async function boot(n: number, opts: { registerMember?: boolean } = {}): Promise { + const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as never); + const engine = new ObjectQL(); + engine.registerDriver(driver as never, true); + await engine.init(); + engine.registerApp({ + id: 'com.objectstack.qa.member-tier-holders-complete', + name: 'member tier holders complete', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: opts.registerMember === false ? [SysOrganization] : [SysOrganization, SysMember], + } as never); + await engine.syncSchemas(); + engines.push(engine); + + const e = engine as any; + await e.insert('sys_organization', { id: ORG, name: ORG, slug: 'large' }, { context: { isSystem: true } }); + await e.insert('sys_organization', { id: ELSEWHERE, name: ELSEWHERE, slug: 'elsewhere' }, { context: { isSystem: true } }); + const holders = Array.from({ length: n }, (_, i) => `usr_${pad(i)}`); + const rows = holders.map((user_id, i) => ({ + id: `mem_${pad((i * 7919) % 1000003)}`, user_id, organization_id: ORG, role: TIER, + })); + rows.push({ id: 'mem_elsewhere', user_id: 'usr_elsewhere', organization_id: ELSEWHERE, role: TIER }); + rows.push({ id: 'mem_other_tier', user_id: 'usr_other_tier', organization_id: ORG, role: 'member' }); + if (opts.registerMember !== false) { + const memberDriver = e.getDriverForObject('sys_member'); + for (let i = 0; i < rows.length; i += 250) await memberDriver.bulkCreate('sys_member', rows.slice(i, i + 250)); + } + + let reads = 0; + (driver as any).knex.on('query', (q: { sql: string }) => { + if (/from [`"]sys_member[`"] where .*[`"]role[`"] = \?/.test(q.sql)) reads += 1; + }); + const warn = vi.fn(); + const svc = new ApprovalService({ + engine: engine as any, + logger: { warn, info: () => {}, error: () => {}, debug: () => {} } as any, + }); + return { engine, svc, warn, holders: [...holders].sort(), tierReads: () => reads }; +} + +/** The private expansions, as the approver-spec resolver calls them. */ +const tier = (rig: Rig, value: string) => (rig.svc as any).expandMembershipTierUsers(value, ORG) as Promise; +const position = (rig: Rig, value: string) => (rig.svc as any).expandPositionUsers(value, ORG) as Promise; +const resolveSpec = (rig: Rig, a: { type: string; value: string }) => + (rig.svc as any).resolveApproverSpec(a, undefined, ORG, Date.now()) as Promise; + +const failMemberReads = (rig: Rig, fault: Error) => { + const memberDriver = (rig.engine as any).getDriverForObject('sys_member'); + return vi.spyOn(memberDriver, 'find').mockRejectedValue(fault); +}; + +describe('[#29362] approvals membership-tier expansion reads every holder (driver-sql, shipped SysMember)', () => { + it('⭐ past the 10,000-row bound: every holder is on the slate, and no one else', async () => { + const rig = await boot(PAGE + 1); + const users = await tier(rig, TIER); + + expect([...users].sort()).toEqual(rig.holders); + expect(users).not.toContain('usr_elsewhere'); + expect(users).not.toContain('usr_other_tier'); + expect(rig.tierReads()).toBe(2); + }, 30_000); + + it('⭐ an org_membership_level approver routes to every holder past the bound', async () => { + const rig = await boot(PAGE + 1); + const slate = await resolveSpec(rig, { type: 'org_membership_level', value: TIER }); + + expect([...slate].sort()).toEqual(rig.holders); + }, 30_000); + + it('control — under the bound: one read, the same slate as before', async () => { + const rig = await boot(PAGE - 1); + const users = await tier(rig, TIER); + + expect([...users].sort()).toEqual(rig.holders); + expect(rig.tierReads()).toBe(1); + }, 30_000); + + it('⛔ a tier read that fails refuses — it never answers an invented empty set', async () => { + const rig = await boot(3); + failMemberReads(rig, Object.assign(new Error('SQLITE_IOERR: disk I/O error'), { code: 'SQLITE_IOERR' })); + + await expect(tier(rig, TIER)).rejects.toThrow(/holders of membership tier 'admin'.*could not be read/); + }); + + it('⛔ a position whose tier limb fails refuses rather than route to the assignment limb alone', async () => { + const rig = await boot(3); + failMemberReads(rig, new Error('SQLITE_IOERR: disk I/O error')); + + await expect(position(rig, TIER)).rejects.toThrow(/could not be read/); + }); + + it('a composition that never registers sys_member holds no tier: [] as before, and the position keeps its literal', async () => { + const rig = await boot(0, { registerMember: false }); + + expect(await tier(rig, TIER)).toEqual([]); + expect(await resolveSpec(rig, { type: 'position', value: TIER })).toEqual([`position:${TIER}`]); + expect(rig.warn.mock.calls.some(([, meta]) => typeof meta?.error === 'string')).toBe(false); + }); + + it('⛔ what the caller sees: the slot falls back to its literal, and the warning names the failed read', async () => { + const rig = await boot(3); + failMemberReads(rig, new Error('SQLITE_IOERR: disk I/O error')); + + expect(await resolveSpec(rig, { type: 'position', value: TIER })).toEqual([`position:${TIER}`]); + expect(await resolveSpec(rig, { type: 'org_membership_level', value: TIER })).toEqual([`org_membership_level:${TIER}`]); + const failed = rig.warn.mock.calls.filter(([, meta]) => typeof meta?.error === 'string'); + expect(failed.map(([, meta]) => meta.type)).toEqual(['position', 'org_membership_level']); + for (const [line, meta] of failed) { + expect(line).toMatch(/could not be expanded — a directory or read-filter lookup failed/); + expect(meta.error).toMatch(/SQLITE_IOERR/); + } + }); +}); diff --git a/packages/plugins/plugin-sharing/src/position-graph.test.ts b/packages/plugins/plugin-sharing/src/position-graph.test.ts index f8ca498fbcd..a1b60745066 100644 --- a/packages/plugins/plugin-sharing/src/position-graph.test.ts +++ b/packages/plugins/plugin-sharing/src/position-graph.test.ts @@ -32,9 +32,11 @@ const ASSIGNMENTS = [ { position: 'sales_rep', user_id: 'u_rep1' }, { position: 'sales_rep', user_id: 'u_rep2' }, ]; +// Every `sys_member` row carries its `id`, as a stored one does: the role read +// pages by it (#29362) and refuses a row it cannot resume after. const MEMBERS = [ - { role: 'sales_rep', user_id: 'u_legacy_rep' }, // better-auth transition source - { role: 'exec', user_id: 'u_exec' }, + { id: 'm_legacy_rep', role: 'sales_rep', user_id: 'u_legacy_rep' }, // better-auth transition source + { id: 'm_exec', role: 'exec', user_id: 'u_exec' }, ]; describe('PositionGraphService (ADR-0090 D3 — flat expansion)', () => { @@ -111,8 +113,8 @@ describe('PositionGraphService.holdsPosition — the user-scoped twin of the exp { position: 'mgr', user_id: 'u_a', organization_id: 'org_a' }, ]; const MEMBER = [ - { role: 'rep', user_id: 'u_role_a', organization_id: 'org_a' }, - { role: 'rep', user_id: 'u_role_b', organization_id: 'org_b' }, + { id: 'm_role_a', role: 'rep', user_id: 'u_role_a', organization_id: 'org_a' }, + { id: 'm_role_b', role: 'rep', user_id: 'u_role_b', organization_id: 'org_b' }, ]; /** Honours every predicate key both reads send: position / role, organization_id, user_id. */ const engine = { diff --git a/packages/plugins/plugin-sharing/src/recipient-width.test.ts b/packages/plugins/plugin-sharing/src/recipient-width.test.ts index 8b9a12f9ce2..5e41ef3bfe5 100644 --- a/packages/plugins/plugin-sharing/src/recipient-width.test.ts +++ b/packages/plugins/plugin-sharing/src/recipient-width.test.ts @@ -583,3 +583,66 @@ describe('#14547 — an org-stamped rule against a SEEDED business unit', () => }); }); }); + +describe('#29362 — the per-record pass is best-effort per rule', () => { + // A rule whose recipients cannot be read now REFUSES (the role read past a + // failed `sys_member` read throws instead of answering nobody). The record's + // write hook reconciles every rule on the object in one pass, so the refusal + // must stay with its own rule: that rule's grants stay as they were, it is + // reported, and the rules after it still reconcile. + const POSITION_RULE = 'share_note_with_sales_reps'; + const USER_RULE = 'share_note_with_auditor'; + + it('the first rule refuses, its grant stays, and the second rule still reconciles', async () => { + const engine = makeEngine(); + const warn = vi.fn(); + const sharing = new SharingService({ engine: engine as any }); + const rules = new SharingRuleService({ engine: engine as any, sharing, logger: { warn, info: vi.fn(), error: vi.fn(), debug: vi.fn() } as any }); + engine.seed('showcase_private_note', [{ id: 'note_1', tag: 'shared', owner_id: 'author' }]); + engine.seed('sys_member', [{ id: 'mem_rep', user_id: 'u_rep', organization_id: null, role: 'sales_rep' }]); + // Listed in this order: the position rule is reconciled FIRST. + engine.seed('sys_sharing_rule', [ + { + id: 'srule_position', organization_id: null, name: POSITION_RULE, + label: 'Note → sales reps', object_name: 'showcase_private_note', + criteria_json: JSON.stringify({ tag: 'shared' }), + recipient_type: 'position', recipient_id: 'sales_rep', + access_level: 'read', active: true, managed_by: 'package', + }, + { + id: 'srule_user', organization_id: null, name: USER_RULE, + label: 'Note → auditor', object_name: 'showcase_private_note', + criteria_json: JSON.stringify({ tag: 'shared' }), + recipient_type: 'user', recipient_id: 'u_auditor', + access_level: 'read', active: true, managed_by: 'package', + }, + ]); + const grantsOf = (sourceId: string) => (engine._tables.sys_record_share ?? []) + .filter((r) => r.record_id === 'note_1' && r.source === 'rule' && r.source_id === sourceId) + .map((r) => String(r.recipient_id)); + + // Both rules materialise while the membership is readable. + await rules.evaluateAllForRecord('showcase_private_note', 'note_1', SYS); + expect(grantsOf('srule_position')).toEqual(['u_rep']); + expect(grantsOf('srule_user')).toEqual(['u_auditor']); + + // The auditor's grant goes missing, and the membership read starts failing. + const shares = engine._tables.sys_record_share!; + shares.splice(shares.findIndex((r) => r.source_id === 'srule_user'), 1); + const find = engine.find.bind(engine); + engine.find = async (o: string, opts?: any) => { + if (o === 'sys_member') throw new Error('SQLITE_IOERR: disk I/O error'); + return find(o, opts); + }; + + const results = await rules.evaluateAllForRecord('showcase_private_note', 'note_1', SYS); + + expect(grantsOf('srule_position')).toEqual(['u_rep']); // not revoked on a read it could not make + expect(grantsOf('srule_user')).toEqual(['u_auditor']); // the next rule still reconciled + expect(results).toHaveLength(1); + const failed = warn.mock.calls.filter(([line]) => line === '[sharing-rule] record reconcile failed for rule'); + expect(failed).toHaveLength(1); + expect(failed[0]![1]).toMatchObject({ rule: POSITION_RULE, recordId: 'note_1' }); + expect(failed[0]![1].error).toMatch(/could not be read/); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 23bbe5b4c39..b9dd282d8a7 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -1410,12 +1410,25 @@ export class SharingRuleService implements ISharingRuleService { // deactivation is honoured immediately. const pass: RuleEvaluationPass = {}; for (const rule of rules) { - // An inactive rule desires nothing; skip the criteria query entirely. - // [#15072] The match read answers with the ROW, not a boolean: for a - // `field` rule the recipients are on it, read by the per-record switch. - const matched = rule.active ? await this.matchRecord(rule, recordId) : null; - const users = matched ? await this.expandRecipientForRecord(rule, matched, pass) : []; - results.push(await this.reconcileForRecord(rule, recordId, matched !== null, users)); + // [#29362] Best-effort per rule, as {@link evaluateAllRulesForObject} is: + // a rule whose recipients cannot be read (a membership read that failed + // now refuses instead of answering nobody) leaves ITS grants as they + // were and is reported, and the record's other rules still reconcile. + try { + // An inactive rule desires nothing; skip the criteria query entirely. + // [#15072] The match read answers with the ROW, not a boolean: for a + // `field` rule the recipients are on it, read by the per-record switch. + const matched = rule.active ? await this.matchRecord(rule, recordId) : null; + const users = matched ? await this.expandRecipientForRecord(rule, matched, pass) : []; + results.push(await this.reconcileForRecord(rule, recordId, matched !== null, users)); + } catch (err: any) { + this.logger?.warn?.('[sharing-rule] record reconcile failed for rule', { + object, + recordId, + rule: rule.name ?? rule.id, + error: err?.message, + }); + } } return results; } diff --git a/packages/plugins/plugin-sharing/src/team-graph-role-holders-complete.test.ts b/packages/plugins/plugin-sharing/src/team-graph-role-holders-complete.test.ts new file mode 100644 index 00000000000..4ad4d3c8eb8 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/team-graph-role-holders-complete.test.ts @@ -0,0 +1,180 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#29362] `TeamGraphService.expandRoleUsers` answers EVERY holder of a role, + * or refuses — never a shortened or an invented empty set. + * + * The role expansion is the `sys_member.role` limb of every position recipient + * (`PositionGraphService.expandPositionUsers`, ADR-0057 D4 transition source) + * and of `expandPrincipal('role')`. It used to read `sys_member` once with a + * 10,000-row bound and take that read as the holder list, and it turned a read + * that threw into `[]`: + * + * - past 10,000 holders the members past the 10,000th row were missing from + * the set with no signal, so a sharing rule granted them nothing; + * - a read that failed answered "nobody holds this role", and the + * retired-recipient revoke (`revokeRuleGrantsForRetiredRecipients`) diffs + * the grants a rule already materialised against that answer — an empty + * answer revokes every one of them. + * + * Measured on a REAL engine (ObjectQL + SqlDriver over in-memory SQLite) + * carrying the shipped `SysMember`, because the completion is a keyset read + * (`id > cursor`, ordered by `id`): a hand-written double that ignores the + * cursor or the order cannot tell a completed read from a repeated page. + * + * A membership object that does not exist is still no holders: a stack that + * never materialises `sys_member` has no role holders to lose. The driver's + * missing-table refusal and the engine's `OBJECT_NOT_FOUND` for `sys_member` + * are the two failures that say so; every other failure is loud. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SysMember, SysOrganization } from '@objectstack/platform-objects/identity'; +import { TeamGraphService } from './team-graph.js'; +import { PositionGraphService } from './position-graph.js'; + +const ORG = 'org_large'; +const ELSEWHERE = 'org_elsewhere'; +const ROLE = 'sales_rep'; +/** The bound the role read is paged at. */ +const PAGE = 10000; + +const pad = (n: number) => String(n).padStart(6, '0'); + +interface Rig { + engine: ObjectQL; + driver: SqlDriver; + /** Every holder of {@link ROLE} in {@link ORG}, sorted. */ + holders: string[]; + /** The role reads the driver ran (`… from sys_member where role = ? …`). */ + roleReads: () => number; +} + +const engines: ObjectQL[] = []; +afterEach(async () => { + vi.restoreAllMocks(); + while (engines.length) { + try { await engines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +/** + * `n` holders of {@link ROLE} in {@link ORG}, plus a holder of the same role in + * another organization and a member of {@link ORG} holding another role — the + * two rows the predicate must keep out. Membership ids are scrambled against + * the user ids, so the 10,001st row by `sys_member.id` is not the 10,001st + * user, and an unordered first read is not the lowest ids. + */ +async function boot(n: number, opts: { registerMember?: boolean } = {}): Promise { + const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as never); + const engine = new ObjectQL(); + engine.registerDriver(driver as never, true); + await engine.init(); + engine.registerApp({ + id: 'com.objectstack.qa.team-graph-role-holders-complete', + name: 'team graph role holders complete', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: opts.registerMember === false ? [SysOrganization] : [SysOrganization, SysMember], + } as never); + await engine.syncSchemas(); + engines.push(engine); + + const e = engine as any; + await e.insert('sys_organization', { id: ORG, name: ORG, slug: 'large' }, { context: { isSystem: true } }); + await e.insert('sys_organization', { id: ELSEWHERE, name: ELSEWHERE, slug: 'elsewhere' }, { context: { isSystem: true } }); + const holders = Array.from({ length: n }, (_, i) => `usr_${pad(i)}`); + if (opts.registerMember !== false) { + const rows = holders.map((user_id, i) => ({ + id: `mem_${pad((i * 7919) % 1000003)}`, user_id, organization_id: ORG, role: ROLE, + })); + rows.push({ id: 'mem_elsewhere', user_id: 'usr_elsewhere', organization_id: ELSEWHERE, role: ROLE }); + rows.push({ id: 'mem_other_role', user_id: 'usr_other_role', organization_id: ORG, role: 'support' }); + const memberDriver = e.getDriverForObject('sys_member'); + for (let i = 0; i < rows.length; i += 250) await memberDriver.bulkCreate('sys_member', rows.slice(i, i + 250)); + } + + let reads = 0; + (driver as any).knex.on('query', (q: { sql: string }) => { + if (/from [`"]sys_member[`"] where .*[`"]role[`"] = \?/.test(q.sql)) reads += 1; + }); + return { engine, driver, holders: [...holders].sort(), roleReads: () => reads }; +} + +const graph = (engine: ObjectQL) => new TeamGraphService({ engine: engine as never, organizationId: ORG }); + +describe('[#29362] TeamGraphService.expandRoleUsers reads every holder (driver-sql, shipped SysMember)', () => { + it('⭐ past the 10,000-row bound: every holder is in the set, and no one else', async () => { + const rig = await boot(PAGE + 1); + const users = await graph(rig.engine).expandRoleUsers(ROLE, ORG); + + expect([...users].sort()).toEqual(rig.holders); + expect(users).not.toContain('usr_elsewhere'); + expect(users).not.toContain('usr_other_role'); + expect(rig.roleReads()).toBe(2); + }, 30_000); + + it('⭐ a position recipient carries the completed role limb', async () => { + const rig = await boot(PAGE + 1); + const team = graph(rig.engine); + const positions = new PositionGraphService({ engine: rig.engine as never, organizationId: ORG, teamGraph: team }); + const users = await positions.expandPositionUsers(ROLE, ORG); + + expect([...users].sort()).toEqual(rig.holders); + }, 30_000); + + it('control — under the bound: one read, the same set as before', async () => { + const rig = await boot(PAGE - 1); + const users = await graph(rig.engine).expandRoleUsers(ROLE, ORG); + + expect([...users].sort()).toEqual(rig.holders); + expect(rig.roleReads()).toBe(1); + }, 30_000); + + it('⛔ a read that fails refuses — it never answers an invented empty set', async () => { + const rig = await boot(3); + const fault = Object.assign(new Error('SQLITE_IOERR: disk I/O error'), { code: 'SQLITE_IOERR' }); + const memberDriver = (rig.engine as any).getDriverForObject('sys_member'); + vi.spyOn(memberDriver, 'find').mockRejectedValue(fault); + + await expect(graph(rig.engine).expandRoleUsers(ROLE, ORG)).rejects.toThrow( + /holders of role 'sales_rep'.*could not be read/, + ); + }); + + it('⛔ a failed role read is not cached: the next read answers the real holders', async () => { + const rig = await boot(3); + const memberDriver = (rig.engine as any).getDriverForObject('sys_member'); + vi.spyOn(memberDriver, 'find').mockRejectedValueOnce(new Error('SQLITE_BUSY: database is locked')); + const team = graph(rig.engine); + + await expect(team.expandRoleUsers(ROLE, ORG)).rejects.toThrow(/could not be read/); + expect([...(await team.expandRoleUsers(ROLE, ORG))].sort()).toEqual(rig.holders); + }); + + it('⛔ a position recipient whose role limb fails refuses rather than answer the assignment limb alone', async () => { + const rig = await boot(3); + const memberDriver = (rig.engine as any).getDriverForObject('sys_member'); + vi.spyOn(memberDriver, 'find').mockRejectedValue(new Error('SQLITE_IOERR: disk I/O error')); + const team = graph(rig.engine); + const positions = new PositionGraphService({ engine: rig.engine as never, organizationId: ORG, teamGraph: team }); + + await expect(positions.expandPositionUsers(ROLE, ORG)).rejects.toThrow(/could not be read/); + }); + + it('a composition that never registers sys_member holds no role: [] as before', async () => { + const rig = await boot(0, { registerMember: false }); + + expect(await graph(rig.engine).expandRoleUsers(ROLE, ORG)).toEqual([]); + }); + + it('a stack whose sys_member table was never created holds no role: [] as before', async () => { + const rig = await boot(0); + await (rig.driver as any).knex.schema.dropTable('sys_member'); + + expect(await graph(rig.engine).expandRoleUsers(ROLE, ORG)).toEqual([]); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/team-graph.ts b/packages/plugins/plugin-sharing/src/team-graph.ts index 4dba66a8381..8e170007a92 100644 --- a/packages/plugins/plugin-sharing/src/team-graph.ts +++ b/packages/plugins/plugin-sharing/src/team-graph.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { ITeamGraphService } from '@objectstack/spec/contracts'; +import { isMissingTableError, keysetWalk } from '@objectstack/types'; import type { SharingEngine } from './sharing-service.js'; const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; @@ -128,6 +129,82 @@ function roleHolderPredicate(roleName: string, org: string | null | undefined): return filter; } +/** + * [#29362] Rows per page of the role-holder read — the bound the read always + * carried, so a role held by fewer people issues the one read it always did. + */ +const ROLE_HOLDER_PAGE_ROWS = 10000; + +/** + * [#29362] Every holder of `roleName` (in `org`, when given), read to + * completion — or a refusal, never a shortened or an invented empty set. + * + * Pages of {@link ROLE_HOLDER_PAGE_ROWS} through the platform's one keyset + * walk (`keysetWalk`, `@objectstack/types`): ordered by `id`, each page + * resuming after the last id the one before it returned, so a role held past + * the bound is read whole rather than cut at an arbitrary 10,000 rows. + * + * Loud, never short. The set feeds sharing GRANTS, and the reconcilers diff the + * grants a rule already holds against it — an answer missing a holder revokes + * that holder's access, and an invented `[]` revokes everyone's. So: + * + * - a read that fails throws, carrying the driver's error as `cause`; + * - a walk that cannot vouch for completion (a row without an `id` to resume + * after, a page the driver returned without honouring the cursor) throws; + * - a membership object that does not exist is the one failure that IS an + * answer — a stack that never materialises `sys_member` has no role + * holders — and answers `[]` as it always did; see + * {@link membershipObjectAbsent}. + */ +/** + * [#29362] Did the role read fail because this stack has no `sys_member` at + * all — the one failure whose empty answer is the truth? Two spellings of the + * same absence, each attributed to `sys_member` so a failure about another + * object is not evidence about this one: the driver's missing-table refusal + * (the shared `isMissingTableError`, the table registered but never created), + * and the engine's `OBJECT_NOT_FOUND` (a composition that never registers the + * object, refused before any driver is asked — the reading + * `sys-metadata-repository.ts` gives its own history object). Anything else + * is an outage and stays loud. + */ +function membershipObjectAbsent(err: unknown): boolean { + if (isMissingTableError(err, 'sys_member')) return true; + const refused = err as { code?: unknown; object?: unknown } | null | undefined; + return refused?.code === 'OBJECT_NOT_FOUND' && refused.object === 'sys_member'; +} + +async function readRoleHoldersToCompletion( + engine: SharingEngine, + roleName: string, + org: string | null | undefined, +): Promise { + const refusal = (reason: string, cause?: unknown) => Object.assign(new Error( + `[sharing] the holders of role '${roleName}'${org ? ` in organization '${org}'` : ''} could not be read: ` + + `${reason}. A shortened or empty holder set would withhold or revoke the grants it confers, so the ` + + 'expansion refuses instead.', + ), cause === undefined ? {} : { cause }); + const users = new Set(); + const walk = keysetWalk>( + (page) => engine.find('sys_member', { ...page, fields: ['id', 'user_id'], context: SYSTEM_CTX }), + { where: roleHolderPredicate(roleName, org), pageSize: ROLE_HOLDER_PAGE_ROWS }, + ); + try { + for await (const page of walk.pages()) { + for (const row of page) { + const uid = String(row?.user_id ?? ''); + if (uid) users.add(uid); + } + } + } catch (err) { + if (membershipObjectAbsent(err)) return []; + throw refusal(err instanceof Error ? err.message : String(err), err); + } + if (walk.truncated) { + throw refusal('the read by id could not continue past a page (a row without an id, or a page that repeated the cursor)'); + } + return Array.from(users); +} + /** * Is `userId` a member of `teamId`? The user-scoped twin of * {@link TeamGraphService.expandUsers}: the same table and the same predicate, @@ -228,18 +305,9 @@ export class TeamGraphService implements ITeamGraphService { const cached = this.cache.expandRole!.get(key); if (cached) return cached; const org = organizationId ?? this.organizationId; - let rows: any[] = []; - try { - rows = await this.engine.find('sys_member', { - filter: roleHolderPredicate(roleName, org), - fields: ['user_id'], - limit: 10000, - context: SYSTEM_CTX, - }); - } catch { - rows = []; - } - const users = Array.from(new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean))); + // [#29362] Read to completion, and a failed read throws — so nothing is + // cached for it, and the next caller reads again. + const users = await readRoleHoldersToCompletion(this.engine, roleName, org); this.cache.expandRole!.set(key, users); return users; }