diff --git a/.changeset/29604-queue-service-refuses-failed-read.md b/.changeset/29604-queue-service-refuses-failed-read.md new file mode 100644 index 00000000000..012338227ae --- /dev/null +++ b/.changeset/29604-queue-service-refuses-failed-read.md @@ -0,0 +1,19 @@ +--- +'@objectstack/plugin-sharing': patch +'@objectstack/plugin-approvals': patch +--- + +fix(plugin-sharing)!: the ownership-queue service refuses a failed member read when the caller asks, and the approver slate and the sharing-recipient grant recompute ask + +Clause-②: yes (narrowing) — `expandMembers` and `isMember` of the registered ownership-queue service, called with `onFailedRead: 'refuse'`, now reject when a read behind the queue fails, where they answered nobody. The approver slate asks for that answer, so an approval node whose `queue` approver cannot be read refuses to open under every `onEmptyApprovers` policy, where it opened on the rest of the slate, on the fallback slate or on the administrators, or auto-approved. The sharing-rule recompute asks for it too, so a whole-rule pass (`evaluateRule`, the REST evaluate endpoint, the boot backfill), the recipient-axis revoke and the per-user pass on a `queue`-recipient rule reject on such a failure, where they completed by revoking the grants of the members who could not be read. Every call without the option, and every read that succeeds, answers as before. +Metadata-protocol: none + + + +**BREAKING** accept-set narrowing at the ownership-queue service's `'refuse'` answer and at its two recompute consumers (`@objectstack/plugin-sharing`, `@objectstack/plugin-approvals`): an empty expansion is a fact, a failed read is a fault (ADR-0142 D2 as amended), and the approver slate (D6) and the sharing-recipient grant recompute (D7) now get the fault instead of "nobody". + +- **The service.** `OwnershipQueueService.expandMembers(name, org, { onFailedRead: 'refuse' })` and `isMember(user, name, org, { onFailedRead: 'refuse' })` reject when the queue's `sys_queue` row, its `sys_queue_member` rows, or a member table one of its specifications reads (`sys_team_member`, `sys_user_position`, `sys_member`, `sys_business_unit_member`, `sys_business_unit`) cannot be read, or a paged read cannot vouch for completion. The rejection's message starts `[queue] queue '' in organization '' does not answer its members, because a read behind it failed:` and names that read; the read's own error rides along as `cause`. It never answers `[]`, a partial set or `false` instead. `isMember` still answers `true` when a read that succeeded reaches the user, and answers `false` only when every read behind it succeeded. An empty queue is still `[]`, and a name that resolves nowhere is still `[]` / `false`. +- **Unchanged: the default.** Without the option (`onFailedRead: 'nobody'`), a failed read still contributes nobody, logged once. The sitting-record access path (`queuesOf`), the claim admission and the assignment pool take that answer, so they deny on a failed read exactly as before. A failed read of the activation ledger (`sys_metadata_activation`) is not a failed read in this sense under either answer: it still reads "switched on" for that call, logged, the posture the sharing-rule service takes for its own position recipients. +- **The approver slate (D6).** A `queue` approver asks with `onFailedRead: 'refuse'` beside the addressing purpose. When the registered service rejects, the node fails with the retryable `APPROVER_DIRECTORY_UNREADABLE:` error that a failed team or position read already raises, naming the queue's failed read, and writes no request, approver or action row. Before, the registered service answered nobody, so the queue added no slot and the node's `onEmptyApprovers` policy acted on a slate missing those people. +- **The sharing-recipient grant recompute (D7).** A `queue`-recipient rule's expansion and its per-user membership check ask with `onFailedRead: 'refuse'`. A failed member read now rejects before any grant is diffed, so the rule's existing grants stay as they were until a pass can read the queue, and each recompute's caller logs it: the record pass and the object pass per rule (`record reconcile failed for rule`, `object reconcile failed for rule`), the queue-membership and business-unit write recomputes (`queue-membership revocation failed`, `queue-membership re-grant failed`, `business-unit revocation failed`, `business-unit re-grant failed`), the membership-row recompute (`recipient membership re-evaluation failed`) and the boot backfill (`boot rule backfill failed for rule`). A direct call to `evaluateRule`, `revokeRuleGrantsForRetiredRecipients` or `reconcileRuleForUser`, the REST evaluate endpoint included, rejects with the service's failure. Before, the failed read answered nobody and the pass revoked the grant of every member behind it. This is the answer a `team` or `position` recipient has given since its own reads began to refuse. +- **Supersedes the approval node's "Not yet".** The approval node's own entry in this release ("an approval node refuses to open when a team or position approver's directory read fails…") says, in its `Clause-②:` line and its **Not yet** bullet, that the registered ownership-queue service answers a failed member read with nobody rather than rejecting, so a `queue` approver backed by it refuses only when `expandMembers` itself rejects. That was true when it was written and is not after this change: the service now rejects on that failure when asked, and the slate asks, so a `queue` approver backed by it refuses on a failed `sys_queue`, `sys_queue_member`, team, position or business-unit read, as a `team` or `position` approver does. Read this bullet in place of those two sentences. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index cbc111d3419..12d19d94639 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -455,15 +455,29 @@ 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. A `team`, - `position`, `department` or `org_membership_level` entry whose holder read - failed (`sys_team_member`, `sys_user_position`, `sys_business_unit_member` - or `sys_member`, and for a `department` also the `sys_business_unit` read of - the unit and of its child units) 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. An `expression` value whose `resolveAs` read failed keeps - its literal and logs the same line, with the `expression` source. + `behavior` that needs every slot (`unanimous`) waits on it too. A + `department` or `org_membership_level` entry whose holder read failed + (`sys_business_unit_member` or `sys_member`, and for a `department` also the + `sys_business_unit` read of the unit and of its child units) 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. An + `expression` value whose `resolveAs: 'department'` read failed keeps its + literal and logs the same line, with the `expression` source. +- **A failed read behind a `team`, `position` or `queue` entry** is not an + empty slate, and the node does not open (ADR-0142 D6). That is a failed + `sys_team_member` or `sys_user_position` read, the `sys_member` read behind a + position, or, for a queue, a failed read of its `sys_queue` row, its + `sys_queue_member` rows or a member table one of its members reads. An + `expression` value resolved as `team` or `position`, and a cross-organization + read filter that fails in front of any of these entries, count the same. + Under every `onEmptyApprovers` value, the `fallbackApprovers` slate included, + the node fails with a retryable error starting `APPROVER_DIRECTORY_UNREADABLE:` + that names the approver and the read, and writes no request. It is not + opened on the rest of the slate, on the fallback or on the administrators, + and it is not auto-approved. Run the flow again once the directory can be + read. A decision on a request opened before approver snapshots existed still + re-reads its slate the old way, which keeps the literal. - **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.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index b656a179d72..db63151a8d1 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -4176,7 +4176,7 @@ describe('ApprovalService — a queue approver routes to the queue\'s members (A const req = await svc.openNodeRequest(queueInput('billing'), CTX) as any; expect(req.pending_approvers).toEqual(['u7', 'u8']); - expect(asked).toEqual([['billing', 't1', { purpose: 'addressing' }]]); + expect(asked).toEqual([['billing', 't1', { purpose: 'addressing', onFailedRead: 'refuse' }]]); expect(warnings.filter(([msg]) => String(msg).includes('expanded to nobody'))).toEqual([]); // A member decides it under their own id. const decided = await svc.decideNode(req.id, { decision: 'approve', actorId: 'u8' }, SYS); @@ -4349,6 +4349,62 @@ describe('ApprovalService — a team, position or queue approver whose directory }); }); +// [ADR-0142 D2 as amended, D6; #29604] The queue arm asks the ownership-queue +// service for its refusal (`onFailedRead: 'refuse'`), beside the ADDRESSING +// purpose. The registered service (`@objectstack/plugin-sharing`) answers a +// failed member read with nobody unless asked, so before the arm asked, a +// queue whose team or position read failed added no slot and the node's +// `onEmptyApprovers` policy opened, rescued or approved on a slate missing its +// people. The double below gives the contract's two answers as that service +// does: a rejection naming the read under `'refuse'`, `[]` under `'nobody'`. +describe('ApprovalService — a queue approver asks the queue service to refuse a failed read, and the node refuses with it (ADR-0142 D6)', () => { + const POLICIES = ['admin_rescue', 'auto_approve', 'fail', 'fallback'] as const; + const READ = "[queue] queue 'billing' in organization 't1' does not answer its members, because a read behind it failed: " + + "the members of team 't_night' could not be read: SQLITE_IOERR: disk I/O error."; + + const withFailingQueue = () => { + const engine = makeFakeEngine(); + const asked: unknown[] = []; + const warnings: any[] = []; + let n = 0; + const svc = new ApprovalService({ + engine, + clock: { now: () => new Date(1757000000000 + (n++) * 1000) }, + logger: { warn: (msg: any, meta: any) => warnings.push([msg, meta]) }, + ownershipQueues: () => ({ + expandMembers: async (name: string, org: string | null, options?: { purpose?: string; onFailedRead?: string }) => { + asked.push([name, org, options]); + if (options?.onFailedRead === 'refuse') throw new Error(READ); + return []; + }, + }), + }); + return { engine, svc, asked, warnings }; + }; + const node = (configExtra: Record) => ({ + ...openInput([]), + config: { approvers: [{ type: 'queue' as const, value: 'billing' }], behavior: 'first_response' as const, lockRecord: false, ...configExtra }, + }); + + for (const p of POLICIES) { + it(`a queue whose member read fails: onEmptyApprovers '${p}' opens nothing and approves nothing`, async () => { + const { engine, svc, asked, warnings } = withFailingQueue(); + const refusal = await svc.openNodeRequest(node( + p === 'fallback' ? { onEmptyApprovers: p, fallbackApprovers: [{ type: 'user', value: 'u_backstop' }] } : { onEmptyApprovers: p }, + ), CTX).then(() => null, (e: unknown) => e); + + expect(asked).toEqual([['billing', 't1', { purpose: 'addressing', onFailedRead: 'refuse' }]]); + expect(refusal).toBeInstanceOf(ApproverDirectoryUnreadableError); + expect(refusal).toMatchObject({ approverType: 'queue', approverValue: 'billing', retryable: true, failedRead: READ }); + expect((refusal as Error).message).toMatch(/^APPROVER_DIRECTORY_UNREADABLE: approver 'queue:billing' could not be expanded in organization 't1'/); + expect(warnings.some(([msg]) => String(msg).includes("approver 'queue:billing' expanded to nobody"))).toBe(false); + expect(engine._tables['sys_approval_request'] ?? []).toEqual([]); + expect(engine._tables['sys_approval_approver'] ?? []).toEqual([]); + expect(engine._tables['sys_approval_action'] ?? []).toEqual([]); + }); + } +}); + // [ADR-0142 D6 as amended; ruling record 6599731698 on #29604, letter B] The // ADR-0105 D9 cross-organization read filter is a read behind a `position` or // `team` approver as much as the directory expansion is. When it fails, the diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index b369ec2a059..c24c61d69ef 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -3482,12 +3482,18 @@ export class ApprovalService implements IApprovalService { * fail-open). The service resolves the name registry-first, then the * organization's own `sys_queue` row, and answers `[]` for a name that * resolves nowhere. With no service registered there is nothing to expand. + * + * [#29604] `onFailedRead: 'refuse'` (D2 as amended): a failed read behind the + * queue rejects instead of answering "nobody", and {@link resolveApproverSpec}'s + * catch turns that rejection into the node's {@link ApproverDirectoryUnreadableError}, + * so the slate is never opened without the people who could not be read. An + * axis of its own beside `purpose`. */ private async expandQueueUsers(queueName: string, organizationId?: string | null): Promise { if (!queueName) return []; const queues = this.queueSource(); if (!queues) return []; - return queues.expandMembers(queueName, organizationId ?? null, { purpose: 'addressing' }); + return queues.expandMembers(queueName, organizationId ?? null, { purpose: 'addressing', onFailedRead: 'refuse' }); } /** diff --git a/packages/plugins/plugin-sharing/src/ownership-queue-service.ts b/packages/plugins/plugin-sharing/src/ownership-queue-service.ts index 8f03d2573a2..f7a8ee4961e 100644 --- a/packages/plugins/plugin-sharing/src/ownership-queue-service.ts +++ b/packages/plugins/plugin-sharing/src/ownership-queue-service.ts @@ -29,14 +29,36 @@ * the union de-duplicated and ordered by user id ascending — so round-robin and * the least-loaded tie-break see one stable order wherever they run. * - * ## Absence is not a fault + * ## Absence is not a fault; a failed read is * * A name that resolves nowhere answers `null` / `[]` / `false`, and an empty - * queue answers `[]`. A failed read of a member table contributes nobody for - * that specification (fail closed for every admission that reads it); a failed - * read of the activation ledger leaves the declared default — on — for that - * call and is logged, the posture the sharing-rule service takes for the same - * ledger. + * queue answers `[]`, under every option: those are facts. + * + * A failed read of a table behind the queue — its `sys_queue` row, its + * `sys_queue_member` rows, or a member table a specification reads (team, + * position, `sys_member`, business unit) — is a fault, and what the call + * answers then is the caller's to say (ADR-0142 D2 as amended, the contract's + * `OwnershipQueueFailedReadAnswer`): + * + * - `onFailedRead: 'nobody'`, the default — what sat behind the failed read + * contributes nobody, logged once. The admission consumers take it, because + * for them nobody denies: the sitting-record access path (D3), the claim + * (D4) and the assignment pool (D5). {@link OwnershipQueueService.queuesOf}, + * which the access path reads, has no option and always answers this way. + * - `onFailedRead: 'refuse'` (#29604) — the call rejects with a failure that + * names the queue, the organization and the read ({@link queueReadRefusal}), + * never `[]`, a partial set or `false`. The recompute consumers take it, + * because for them nobody revokes: the approver slate (D6) and the + * sharing-recipient grant recompute (D7). `isMember` under it still answers + * `true` when a read that succeeded reaches the user — a failed read beside + * it cannot take that membership away — and `false` only when every read + * behind the answer succeeded. + * + * A failed read of the activation ledger is neither: under either answer it + * leaves the declared default — on — for that call and is logged, the posture + * the sharing-rule service takes for the same ledger on its own position + * recipients, so a queue's position and a rule's position never disagree on + * an outage (#29604 keeps it; the contract leaves it to this service). * * ## Read to completion * @@ -46,13 +68,13 @@ * (`readMembershipToCompletion`), never cut at a row bound: a queue past the * bound lost its later members, a member past it lost every queue it sits in, * and nothing said so. A read that fails, or a walk that cannot vouch for - * completion, takes the failed-read answer above. ADR-0142 D2 as amended has - * the recompute consumers (D6, D7) refuse a failed read instead; that answer - * waits on the contract carrying the caller's call-time signal. + * completion, is a failed read above, never an answer on the pages already + * read. */ import type { IOwnershipQueueService, + OwnershipQueueFailedReadAnswer, QueueExpansionPurpose, QueueMemberSpec, ResolvedQueue, @@ -117,19 +139,47 @@ function isRecord(v: unknown): v is Record { return v !== null && typeof v === 'object' && !Array.isArray(v); } +/** A walk's or a holder check's closing consequence ("…, so the expansion refuses instead."). */ +const WALK_CONSEQUENCE = / [^.]*\brefuses instead\.$/; + /** * The reason behind a failed authored read, for a warning that answers nobody. * A walk's refusal ends on its own consequence ("…, so the expansion refuses - * instead."), which is not this service's answer until #29604 part 2, so only - * the reason is logged: the driver's error the refusal carries as `cause`, or, - * for a walk that could not vouch for completion (no cause), the refusal's - * sentence with that closing clause cut. + * instead."), which is not the answer a call under `onFailedRead: 'nobody'` + * gives, so only the reason is logged: the driver's error the refusal carries + * as `cause`, or, for a walk that could not vouch for completion (no cause), + * the refusal's sentence with that closing clause cut. */ function readFailureReason(err: unknown): string { const cause = (err as { cause?: unknown } | null)?.cause; if (cause instanceof Error) return cause.message; const message = err instanceof Error ? err.message : String(err); - return message.replace(/ [^.]*\brefuses instead\.$/, ''); + return message.replace(WALK_CONSEQUENCE, ''); +} + +/** + * [#29604] The failure a call under `onFailedRead: 'refuse'` rejects with: the + * queue, its organization, and the read that failed, in the read's own words — + * a membership walk's or a holder check's refusal with its `[sharing]` tag and + * its closing consequence cut (the consequence here is this sentence's), or + * this service's own sentence for the `sys_queue` row read. The failed read's + * error rides along as `cause`. + */ +function queueReadRefusal(name: string, organizationId: string | null, err: unknown): Error { + const read = (err instanceof Error ? err.message : String(err)) + .replace(/^\[sharing\] /, '') + .replace(WALK_CONSEQUENCE, '') + .trim(); + const inOrg = organizationId ? ` in organization '${organizationId}'` : ''; + return Object.assign( + new Error( + `[queue] queue '${name}'${inOrg} does not answer its members, because a read behind it failed: ` + + `${read}${/[.!?]$/.test(read) ? '' : '.'} The caller asked for a refusal over a partial answer ` + + '(onFailedRead \'refuse\'): answering with the members that could be read would revoke or route past ' + + 'the ones that could not.', + ), + { cause: err }, + ); } /** @@ -140,12 +190,21 @@ function readFailureReason(err: unknown): string { interface ExpansionPass { /** `'addressing'` keeps a switched-off position's holders (see the contract's `expandMembers`). */ purpose: QueueExpansionPurpose; + /** + * [#29604] `onFailedRead: 'refuse'`: a failed member-table read rejects the + * call instead of contributing nobody. An axis of its own, never read off + * {@link purpose}. + */ + refuse: boolean; positionActive: Map; expansions: Map>; } -function newPass(purpose: QueueExpansionPurpose = 'access'): ExpansionPass { - return { purpose, positionActive: new Map(), expansions: new Map() }; +function newPass( + purpose: QueueExpansionPurpose = 'access', + onFailedRead: OwnershipQueueFailedReadAnswer = 'nobody', +): ExpansionPass { + return { purpose, refuse: onFailedRead === 'refuse', positionActive: new Map(), expansions: new Map() }; } export class OwnershipQueueService implements IOwnershipQueueService { @@ -161,11 +220,21 @@ export class OwnershipQueueService implements IOwnershipQueueService { // ── The contract ─────────────────────────────────────────────────────────── async resolveQueue(name: string, organizationId: string | null): Promise { + return this.resolve(name, organizationId, false); + } + + /** + * {@link resolveQueue}, with the caller's failed-read answer: under `refuse` + * a failed `sys_queue` or `sys_queue_member` read rejects (a failure naming + * the read, which the public caller wraps in {@link queueReadRefusal}) + * instead of resolving nowhere or to no authored members. + */ + private async resolve(name: string, organizationId: string | null, refuse: boolean): Promise { const engine = this.engine(); if (!engine || !name) return null; const declared = await this.declaredQueueItem(name); if (declared) { - const authoredMembers = await this.authoredMemberSpecs(engine, [name], organizationId); + const authoredMembers = await this.authoredMemberSpecs(engine, [name], organizationId, refuse); return { name, label: declared.label ?? name, @@ -180,9 +249,9 @@ export class OwnershipQueueService implements IOwnershipQueueService { // A declared name switched off in the ledger resolves nowhere — not even to // an authored row, which may not take a declared queue's name. if (this.declaredItems().some((q) => q.name === name)) return null; - const row = await this.authoredQueueRow(engine, name, organizationId); + const row = await this.authoredQueueRow(engine, name, organizationId, refuse); if (!row) return null; - const authoredMembers = await this.authoredMemberSpecs(engine, [name], organizationId); + const authoredMembers = await this.authoredMemberSpecs(engine, [name], organizationId, refuse); return { name, label: str(row.label) ?? name, @@ -195,18 +264,37 @@ export class OwnershipQueueService implements IOwnershipQueueService { async expandMembers( name: string, organizationId: string | null, - options?: { purpose?: QueueExpansionPurpose }, + options?: { purpose?: QueueExpansionPurpose; onFailedRead?: OwnershipQueueFailedReadAnswer }, ): Promise { - const queue = await this.resolveQueue(name, organizationId); - if (!queue) return []; - return this.expandSpecs(queue.members, organizationId, newPass(options?.purpose)); + const pass = newPass(options?.purpose, options?.onFailedRead); + try { + const queue = await this.resolve(name, organizationId, pass.refuse); + if (!queue) return []; + return await this.expandSpecs(queue.members, organizationId, pass); + } catch (err) { + // Only a call under 'refuse' gets here: under 'nobody' every read + // behind the queue answers nobody in place (module header). + if (!pass.refuse) throw err; + throw queueReadRefusal(name, organizationId, err); + } } - async isMember(userId: string, name: string, organizationId: string | null): Promise { + async isMember( + userId: string, + name: string, + organizationId: string | null, + options?: { onFailedRead?: OwnershipQueueFailedReadAnswer }, + ): Promise { if (!userId) return false; - const queue = await this.resolveQueue(name, organizationId); - if (!queue) return false; - return this.specsReachUser(queue.members, userId, organizationId, newPass()); + const pass = newPass('access', options?.onFailedRead); + try { + const queue = await this.resolve(name, organizationId, pass.refuse); + if (!queue) return false; + return await this.specsReachUser(queue.members, userId, organizationId, pass); + } catch (err) { + if (!pass.refuse) throw err; + throw queueReadRefusal(name, organizationId, err); + } } async queuesOf(userId: string, organizationId: string | null): Promise { @@ -331,6 +419,9 @@ export class OwnershipQueueService implements IOwnershipQueueService { let pending = pass.expansions.get(key); if (!pending) { pending = this.expandSpecUncached(spec, organizationId, pass).catch((err: any) => { + // [#29604] Under 'refuse' the specification's failed read is the + // call's answer — the pass, and so this rejected memo, ends with it. + if (pass.refuse) throw err; this.warnOnce(`expand::${key}`, '[queue] a member specification could not be expanded — it contributes nobody until it can', { type: spec.type, value: spec.value, error: err?.message, }); @@ -373,6 +464,14 @@ export class OwnershipQueueService implements IOwnershipQueueService { * {@link expandSpecs}: a position or a team is answered by an indexed * holder check rather than a full expansion, the reason * `PositionGraphService.holdsPosition` exists. + * + * [#29604] Under `onFailedRead: 'refuse'` a specification whose read fails + * does not end the walk: a later one that reaches the user still answers + * `true`, because a read that succeeded and found the user is a fact no + * failure beside it can take away. Only when none reaches does the first + * failure become the answer — `false` is given only when every read behind it + * succeeded. Under `'nobody'` no specification rejects (each failed read + * contributes nobody in place), so the walk is the one it always was. */ private async specsReachUser( specs: readonly QueueMemberSpec[], @@ -382,44 +481,56 @@ export class OwnershipQueueService implements IOwnershipQueueService { ): Promise { const engine = this.engine(); if (!engine) return false; + let failed: { err: unknown } | undefined; for (const spec of specs) { if (!spec.value) continue; - switch (spec.type) { - case 'user': - if (spec.value === userId) return true; - break; - case 'team': - if (await this.holderCheck(spec, () => teamHasMember(engine, spec.value, userId))) return true; - break; - case 'position': { - if (!(await this.positionActive(spec.value, pass))) break; - const positions = new PositionGraphService({ engine, organizationId }); - if (await this.holderCheck(spec, () => positions.holdsPosition(userId, spec.value, organizationId ?? undefined))) return true; - break; - } - default: - if ((await this.expandSpec(spec, organizationId, pass)).includes(userId)) return true; + try { + if (await this.specReachesUser(engine, spec, userId, organizationId, pass)) return true; + } catch (err) { + if (!pass.refuse) throw err; + failed ??= { err }; } } + if (failed) throw failed.err; return false; } + private async specReachesUser( + engine: OwnershipQueueEngine, + spec: QueueMemberSpec, + userId: string, + organizationId: string | null, + pass: ExpansionPass, + ): Promise { + switch (spec.type) { + case 'user': + return spec.value === userId; + case 'team': + return this.holderCheck(spec, pass, () => teamHasMember(engine, spec.value, userId)); + case 'position': { + if (!(await this.positionActive(spec.value, pass))) return false; + const positions = new PositionGraphService({ engine, organizationId }); + return this.holderCheck(spec, pass, () => positions.holdsPosition(userId, spec.value, organizationId ?? undefined)); + } + default: + return (await this.expandSpec(spec, organizationId, pass)).includes(userId); + } + } + /** - * [#29503] A team or position holder check, answered in the posture this - * service declares for a failed member-table read (module header): that - * specification contributes nobody, logged once — the same answer - * {@link expandSpec} gives the expansion, so the two twins cannot disagree. - * - * The checks themselves (`teamHasMember`, `PositionGraphService.holdsPosition`) - * now refuse a failed read instead of answering `false`; this keeps the - * queue's consumers where they were. ADR-0142 D2 as amended (#29604) has the - * recompute consumers refuse instead; the call-time signal they say so with - * is the contract's to carry, and until it does every caller gets this answer. + * [#29503] A team or position holder check, answered in the posture the + * call asked for (module header). The checks themselves (`teamHasMember`, + * `PositionGraphService.holdsPosition`) refuse a failed read instead of + * answering `false`. Under `onFailedRead: 'nobody'` that specification + * contributes nobody, logged once — the same answer {@link expandSpec} gives + * the expansion, so the two twins cannot disagree. Under `'refuse'` (#29604) + * the check's refusal is passed on, as the expansion's is. */ - private async holderCheck(spec: QueueMemberSpec, check: () => Promise): Promise { + private async holderCheck(spec: QueueMemberSpec, pass: ExpansionPass, check: () => Promise): Promise { try { return await check(); } catch (err: any) { + if (pass.refuse) throw err; this.warnOnce(`holds::${spec.type}::${spec.value}`, '[queue] a member specification could not be read for a holder check — it contributes nobody until it can', { type: spec.type, value: spec.value, error: err?.message, }); @@ -442,10 +553,16 @@ export class OwnershipQueueService implements IOwnershipQueueService { return items.filter((q) => !off.has(q.name)); } + /** + * The organization's `sys_queue` row named `name`. A failed read resolves + * the name nowhere, logged once — or, under `refuse` (#29604), rejects with + * a sentence naming the read: whether the queue exists is itself unknown. + */ private async authoredQueueRow( engine: OwnershipQueueEngine, name: string, organizationId: string | null, + refuse = false, ): Promise | null> { try { const rows = await engine.find(QUEUE_OBJECT, { @@ -456,6 +573,12 @@ export class OwnershipQueueService implements IOwnershipQueueService { }); return Array.isArray(rows) && isRecord(rows[0]) ? rows[0] : null; } catch (err: any) { + if (refuse) { + throw Object.assign( + new Error(`the ${QUEUE_OBJECT} row of queue '${name}'${organizationId ? ` in organization '${organizationId}'` : ''} could not be read: ${err?.message ?? String(err)}`), + { cause: err }, + ); + } this.warnOnce('read::sys_queue', '[queue] sys_queue read failed — authored queues resolve nowhere until it succeeds', { error: err?.message }); return null; } @@ -487,11 +610,16 @@ export class OwnershipQueueService implements IOwnershipQueueService { } } - /** The organization's `sys_queue_member` rows for `names`, grouped by queue. */ + /** + * The organization's `sys_queue_member` rows for `names`, grouped by queue. + * A failed read counts no authored member, logged once — or, under `refuse` + * (#29604), passes the walk's refusal on, which names the read. + */ private async authoredMemberSpecs( engine: OwnershipQueueEngine, names: readonly string[], organizationId: string | null, + refuse = false, ): Promise> { const out = new Map(); if (names.length === 0) return out; @@ -515,6 +643,7 @@ export class OwnershipQueueService implements IOwnershipQueueService { out.set(queue, list); } } catch (err: any) { + if (refuse) throw err; this.warnOnce('read::sys_queue_member', '[queue] sys_queue_member read failed — authored members count as nobody until it succeeds', { error: readFailureReason(err) }); } return out; diff --git a/packages/plugins/plugin-sharing/src/ownership-queue.test.ts b/packages/plugins/plugin-sharing/src/ownership-queue.test.ts index f5d8402d6cb..134c727e42d 100644 --- a/packages/plugins/plugin-sharing/src/ownership-queue.test.ts +++ b/packages/plugins/plugin-sharing/src/ownership-queue.test.ts @@ -208,11 +208,12 @@ describe('[ADR-0142 D2] OwnershipQueueService — the one expander', () => { }); it('[#29503] a failed team or position holder read keeps the queue\'s declared posture: that specification contributes nobody, logged once', async () => { - // The team and position reads now refuse a failed read instead of answering - // "nobody"; the queue service keeps answering for its own consumers exactly - // as its header declares (a failed member-table read contributes nobody), - // on the expansion and on the holder check alike. Whether the queue should - // refuse too is left to a ruling (ADR-0142 does not declare this posture). + // The team and position reads refuse a failed read instead of answering + // "nobody"; under the default `onFailedRead: 'nobody'` the queue service + // answers its admission consumers as its header declares (a failed + // member-table read contributes nobody), on the expansion and on the + // holder check alike. Under `'refuse'` it rejects instead — pinned in the + // `onFailedRead 'refuse'` describe below (#29604). for (const failed of ['sys_team_member', 'sys_user_position'] as const) { const warn = vi.fn(); const svc = service(makeEngine({ declared: SUPPORT_DECLARED, tables: BASE_TABLES(), failOn: [failed] }), { warn }); @@ -387,6 +388,95 @@ describe('[ADR-0142 D2] OwnershipQueueService — the queue reads page to comple }); }); +describe('[ADR-0142 D2 as amended] OwnershipQueueService — onFailedRead \'refuse\' rejects a failed read, naming it (#29604)', () => { + const REFUSE = { onFailedRead: 'refuse' } as const; + const esc = (text: string) => text.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + /** The refusal's shape: the queue, its organization, then the read that failed, never the walk's own consequence. */ + const refusal = (queue: string, read: string) => + new RegExp(`^${esc(`[queue] queue '${queue}' in organization 'org_a' does not answer its members, because a read behind it failed: ${read}`)}`); + + it('⛔ a failed sys_queue row read rejects, naming it — the default still resolves the name nowhere', async () => { + const warn = vi.fn(); + const svc = service(makeEngine({ tables: BASE_TABLES(), failOn: ['sys_queue'] }), { warn }); + const read = "the sys_queue row of queue 'billing' in organization 'org_a' could not be read: sys_queue unavailable."; + await expect(svc.expandMembers('billing', 'org_a', REFUSE)).rejects.toThrow(refusal('billing', read)); + await expect(svc.isMember('u_e', 'billing', 'org_a', REFUSE)).rejects.toThrow(refusal('billing', read)); + expect(warn).not.toHaveBeenCalled(); + // The admission answer, unchanged: nobody, logged once. + expect(await svc.expandMembers('billing', 'org_a')).toEqual([]); + expect(await svc.isMember('u_e', 'billing', 'org_a')).toBe(false); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('⛔ a failed sys_queue_member read rejects — never the declared positions alone', async () => { + const svc = service(makeEngine({ declared: SUPPORT_DECLARED, tables: BASE_TABLES(), failOn: ['sys_queue_member'] }), { warn: vi.fn() }); + await expect(svc.expandMembers('support_tier1', 'org_a', { purpose: 'addressing', ...REFUSE })).rejects.toThrow( + refusal('support_tier1', "the member rows of queue 'support_tier1' in organization 'org_a' could not be read: sys_queue_member unavailable. The caller"), + ); + await expect(svc.isMember('u_d', 'support_tier1', 'org_a', REFUSE)).rejects.toThrow(/sys_queue_member unavailable/); + expect(await svc.expandMembers('support_tier1', 'org_a')).toEqual(['u_a', 'u_b']); + }); + + it('⛔ a failed member-table read behind a specification rejects, naming the read and carrying the driver error as cause', async () => { + // support_tier1 = position support_agent (u_a, u_b; its sys_member limb + // too) + team t_night (u_a, u_d); billing = position billing_clerk (u_c) + user u_e. + for (const [failed, queue, read] of [ + ['sys_team_member', 'support_tier1', "the members of team 't_night' could not be read: sys_team_member unavailable"], + ['sys_user_position', 'support_tier1', "the assignments of position 'support_agent' in organization 'org_a' could not be read: sys_user_position unavailable"], + ['sys_member', 'billing', "the holders of role 'billing_clerk' in organization 'org_a' could not be read: sys_member unavailable"], + ] as const) { + const svc = service(makeEngine({ declared: SUPPORT_DECLARED, tables: BASE_TABLES(), failOn: [failed] }), { warn: vi.fn() }); + const err = await svc.expandMembers(queue, 'org_a', REFUSE).then(() => null, (e: unknown) => e as Error); + expect(err?.message, failed).toMatch(refusal(queue, `${read}. The caller`)); + expect(err?.message, failed).not.toMatch(/refuses instead/); + expect(((err as any)?.cause as Error)?.message, failed).toMatch(new RegExp(`${failed} unavailable`)); + } + }); + + it('isMember under refuse: true when a read that succeeded reaches the user, whichever specification failed first; a rejection when none does', async () => { + // Position first (declared), team second (authored). + const positionDown = service(makeEngine({ declared: SUPPORT_DECLARED, tables: BASE_TABLES(), failOn: ['sys_user_position'] }), { warn: vi.fn() }); + expect(await positionDown.isMember('u_d', 'support_tier1', 'org_a', REFUSE)).toBe(true); // the team reaches u_d after the failure + await expect(positionDown.isMember('u_b', 'support_tier1', 'org_a', REFUSE)).rejects.toThrow(/sys_user_position unavailable/); + await expect(positionDown.isMember('u_z', 'support_tier1', 'org_a', REFUSE)).rejects.toThrow(/sys_user_position unavailable/); + + const teamDown = service(makeEngine({ declared: SUPPORT_DECLARED, tables: BASE_TABLES(), failOn: ['sys_team_member'] }), { warn: vi.fn() }); + expect(await teamDown.isMember('u_b', 'support_tier1', 'org_a', REFUSE)).toBe(true); // the position reaches u_b before it + await expect(teamDown.isMember('u_d', 'support_tier1', 'org_a', REFUSE)).rejects.toThrow( + refusal('support_tier1', "whether user 'u_d' is a member of team 't_night' could not be read: sys_team_member unavailable. The caller"), + ); + // The admission answer for the same user, unchanged. + expect(await teamDown.isMember('u_d', 'support_tier1', 'org_a')).toBe(false); + }); + + it('facts stay facts: an empty queue is [], an unresolved name is [] / false, and healthy reads answer what the default answers', async () => { + const svc = service(makeEngine({ declared: [...SUPPORT_DECLARED, { name: 'empty_q', label: 'Empty', members: [] }], tables: BASE_TABLES() })); + expect(await svc.expandMembers('empty_q', 'org_a', REFUSE)).toEqual([]); + expect(await svc.expandMembers('nowhere', 'org_a', REFUSE)).toEqual([]); + expect(await svc.isMember('u_a', 'nowhere', 'org_a', REFUSE)).toBe(false); + expect(await svc.expandMembers('support_tier1', 'org_a', REFUSE)).toEqual(['u_a', 'u_b', 'u_d']); + expect(await svc.expandMembers('billing', 'org_a', REFUSE)).toEqual(['u_c', 'u_e']); + expect(await svc.isMember('u_z', 'support_tier1', 'org_a', REFUSE)).toBe(false); + expect(await svc.isMember('u_d', 'support_tier1', 'org_a', REFUSE)).toBe(true); + }); + + it('a refusal is not remembered: the next call, once the table reads, answers the members', async () => { + const failOn = ['sys_team_member']; + const svc = service(makeEngine({ declared: SUPPORT_DECLARED, tables: BASE_TABLES(), failOn }), { warn: vi.fn() }); + await expect(svc.expandMembers('support_tier1', 'org_a', REFUSE)).rejects.toThrow(/sys_team_member unavailable/); + failOn.length = 0; + expect(await svc.expandMembers('support_tier1', 'org_a', REFUSE)).toEqual(['u_a', 'u_b', 'u_d']); + }); + + it('a failed activation-ledger read is not a failed read under refuse either: the declared default — on — answers, logged', async () => { + const warn = vi.fn(); + const svc = service(makeEngine({ declared: SUPPORT_DECLARED, tables: BASE_TABLES(), failOn: [METADATA_ACTIVATION_TABLE] }), { warn }); + expect(await svc.expandMembers('support_tier1', 'org_a', REFUSE)).toEqual(['u_a', 'u_b', 'u_d']); + expect(await svc.isMember('u_b', 'support_tier1', 'org_a', REFUSE)).toBe(true); + expect(warn.mock.calls.some(([line]) => /treated as switched ON/.test(String(line)))).toBe(true); + }); +}); + describe('[ADR-0131 D4] OwnershipQueueService.hasAnyQueue — the session payload\'s one boolean', () => { const ROW_ONLY = (): Record => ({ sys_queue: [{ id: 'q1', name: 'billing', label: 'Billing', organization_id: 'org_a' }], @@ -533,6 +623,22 @@ describe('[ADR-0142 D3] SharingService — a record sitting in a queue the calle expect(JSON.stringify(await bare.buildReadFilter('ticket', ctx('u_a')))).not.toContain('"queue"'); }); + it('[#29604] control — a failed member read reaches nobody through the queue and refuses nothing: the access path keeps the default answer', async () => { + const down = makeEngine({ + declared: SUPPORT_DECLARED, + tables: engine._tables, + schemas: { ticket: TICKET_SCHEMA, job: AUTHOR_QUEUE_SCHEMA, idea: OPEN_SCHEMA, sys_record_share: { name: 'sys_record_share' } }, + failOn: ['sys_team_member'], + }); + const queues = service(down, { warn: vi.fn() }); + const s = new SharingService({ engine: down as any, ownershipQueues: () => queues }); + // u_d sits in support_tier1 only through team t_night, whose read fails: no access, and no throw. + expect(await s.checkEdit('ticket', 't1', ctx('u_d'))).toBe('deny'); + expect(JSON.stringify(await s.buildReadFilter('ticket', ctx('u_d')) ?? {})).not.toContain('"queue"'); + // u_b sits in it through position support_agent, which reads. + expect(await s.checkEdit('ticket', 't1', ctx('u_b'))).toBe('allow'); + }); + it('membership is resolved once per request context', async () => { const queuesOf = vi.fn(async () => ['support_tier1']); const s = new SharingService({ engine: engine as any, ownershipQueues: () => ({ queuesOf, isMember: async () => false }) }); diff --git a/packages/plugins/plugin-sharing/src/queue-recipient.test.ts b/packages/plugins/plugin-sharing/src/queue-recipient.test.ts index 85b68de80a9..1b642f85f7d 100644 --- a/packages/plugins/plugin-sharing/src/queue-recipient.test.ts +++ b/packages/plugins/plugin-sharing/src/queue-recipient.test.ts @@ -10,6 +10,7 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; import { SharingService } from './sharing-service.js'; import { SharingRuleService } from './sharing-rule-service.js'; +import { OwnershipQueueService } from './ownership-queue-service.js'; interface Row { [k: string]: any } @@ -44,12 +45,16 @@ function matches(row: Row, f: any): boolean { function makeEngine() { const tables: Record = {}; const ensure = (n: string) => (tables[n] ??= []); + /** Tables whose reads fail, as an outage of that table does (#29604). */ + const failing = new Set(); let seq = 0; return { _tables: tables, + failing, getSchema() { return undefined; }, seed(object: string, rows: Row[]) { ensure(object).push(...rows.map((r) => ({ ...r }))); }, async find(o: string, opts?: any) { + if (failing.has(o)) throw new Error(`${o} unavailable`); const f = opts?.filter ?? opts?.where; return ensure(o).filter((r) => matches(r, f)).slice(0, opts?.limit ?? 10000); }, @@ -110,7 +115,8 @@ describe('[ADR-0142 D7] the queue recipient', () => { }); await rules.evaluateRule(RULE, SYS); expect(granteesOf('inv1')).toEqual(['u_c', 'u_e']); - expect(expandMembers).toHaveBeenCalledWith('billing', 'org_a'); + // [#29604] On the recompute answer: a failed member read refuses, never "nobody". + expect(expandMembers).toHaveBeenCalledWith('billing', 'org_a', { onFailedRead: 'refuse' }); }); it('a member who left loses the grant on the next recompute', async () => { @@ -141,3 +147,95 @@ describe('[ADR-0142 D7] the queue recipient', () => { } as any, SYS)).rejects.toThrow(/machine name/); }); }); + +/** + * [ADR-0142 D2 as amended, D7; #29604] The grant recompute asks the queue + * service for its refusal, so a member read that fails leaves the rule's + * grants as they were — the whole-rule pass, the recipient-axis revoke and the + * per-user pass alike — where "nobody" revoked every grant behind the failed + * read. Driven through the registered `OwnershipQueueService` over the same + * engine, so the refusal is the service's own, not a stub's. + */ +describe('[ADR-0142 D7] a failed queue-member read keeps the queue-recipient rule\'s grants (#29604)', () => { + let engine: ReturnType; + type Warn = (msg: string, meta?: Record) => void; + let warn: ReturnType>; + let rules: SharingRuleService; + const REFUSAL = /\[queue\] queue 'billing' in organization 'org_a' does not answer its members, because a read behind it failed/; + const granteesOf = (recordId: string): string[] => + (engine._tables.sys_record_share ?? []) + .filter((r) => r.record_id === recordId && r.source === 'rule') + .map((r) => String(r.recipient_id)) + .sort(); + const theRule = async () => (await rules.listRules({ activeOnly: true }, SYS))[0]; + + beforeEach(async () => { + engine = makeEngine(); + engine.seed('invoice', [{ id: 'inv1', status: 'open', owner_id: 'author', organization_id: 'org_a' }]); + engine.seed('sys_sharing_rule', [{ + id: 'srule_q', organization_id: 'org_a', name: RULE, label: 'Invoices → Billing queue', + object_name: 'invoice', criteria_json: JSON.stringify({ status: 'open' }), + recipient_type: 'queue', recipient_id: 'billing', access_level: 'read', active: true, + }]); + // billing = user u_c + team t_night (u_d). + engine.seed('sys_queue', [{ id: 'q1', name: 'billing', label: 'Billing', organization_id: 'org_a' }]); + engine.seed('sys_queue_member', [ + { id: 'qm_1', queue: 'billing', recipient_type: 'user', recipient_id: 'u_c', organization_id: 'org_a' }, + { id: 'qm_2', queue: 'billing', recipient_type: 'team', recipient_id: 't_night', organization_id: 'org_a' }, + ]); + engine.seed('sys_team_member', [{ id: 'tm_1', team_id: 't_night', user_id: 'u_d' }]); + warn = vi.fn(); + const logger = { warn, info: vi.fn() }; + const queues = new OwnershipQueueService({ engine: () => engine as any, logger }); + rules = new SharingRuleService({ + engine: engine as any, + sharing: new SharingService({ engine: engine as any }), + ownershipQueues: () => queues, + logger, + }); + await rules.evaluateRule(RULE, SYS); + expect(granteesOf('inv1')).toEqual(['u_c', 'u_d']); + }); + + it('⛔ the whole-rule pass refuses and revokes nothing — the queue\'s own member rows or a member table', async () => { + for (const table of ['sys_queue_member', 'sys_team_member']) { + engine.failing.add(table); + await expect(rules.evaluateRule(RULE, SYS), table).rejects.toThrow(REFUSAL); + expect(granteesOf('inv1'), table).toEqual(['u_c', 'u_d']); + engine.failing.delete(table); + } + }); + + it('⛔ the recipient-axis revoke refuses and retires nobody', async () => { + engine.failing.add('sys_team_member'); + await expect(rules.revokeRuleGrantsForRetiredRecipients(await theRule())).rejects.toThrow(REFUSAL); + expect(granteesOf('inv1')).toEqual(['u_c', 'u_d']); + }); + + it('⛔ the per-user pass refuses for a member it cannot read, and still answers the member it can', async () => { + engine.failing.add('sys_team_member'); + const rule = await theRule(); + await expect(rules.reconcileRuleForUser(rule, 'u_d')).rejects.toThrow(REFUSAL); + expect(granteesOf('inv1')).toEqual(['u_c', 'u_d']); + // u_c is reached by a read that succeeded: a fact, answered as one. + await expect(rules.reconcileRuleForUser(rule, 'u_c')).resolves.toMatchObject({ grantsRevoked: 0 }); + expect(granteesOf('inv1')).toEqual(['u_c', 'u_d']); + }); + + it('⛔ the record pass keeps the rule\'s grants and reports it, per rule (#29362)', async () => { + engine.failing.add('sys_team_member'); + await expect(rules.evaluateAllForRecord('invoice', 'inv1', SYS)).resolves.toEqual([]); + expect(granteesOf('inv1')).toEqual(['u_c', 'u_d']); + expect(warn.mock.calls.some(([line, meta]) => + /record reconcile failed for rule/.test(String(line)) && REFUSAL.test(String((meta as any)?.error)))).toBe(true); + }); + + it('after the refusal, once the read succeeds, the next recompute converges: a member who left loses the grant', async () => { + engine.failing.add('sys_team_member'); + await expect(rules.evaluateRule(RULE, SYS)).rejects.toThrow(REFUSAL); + engine.failing.delete('sys_team_member'); + engine._tables.sys_team_member.splice(0); + await rules.evaluateRule(RULE, SYS); + expect(granteesOf('inv1')).toEqual(['u_c']); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index b9dd282d8a7..3e9f1cb56ae 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -1741,10 +1741,16 @@ export class SharingRuleService implements ISharingRuleService { } if (rule.recipient_type === 'team') return teamHasMember(this.engine, rule.recipient_id, userId); // [ADR-0142 D7] A queue reaches the user its specifications reach — the - // queue service's holder check, not a full expansion. + // queue service's holder check, not a full expansion. Under + // `onFailedRead: 'refuse'` (D2 as amended, #29604): a failed member read + // rejects rather than answering "not a member", which would revoke the + // user's grants; the per-user pass's caller catches it per rule and the + // grants stay, as they do for a team or position recipient (#29503). if (rule.recipient_type === 'queue') { const queues = this.ownershipQueues?.(); - return queues ? queues.isMember(userId, rule.recipient_id, rule.organization_id ?? null) : false; + return queues + ? queues.isMember(userId, rule.recipient_id, rule.organization_id ?? null, { onFailedRead: 'refuse' }) + : false; } return (await this.expandRecipient(rule, pass)).includes(userId); } @@ -2181,9 +2187,15 @@ export class SharingRuleService implements ISharingRuleService { // the two recompute modules' look-through). ⛔ This grants the rule's // matched records to the queue's people; it confers no sitting-record // access, which is the access path's (`SharingService`). + // + // [#29604] `onFailedRead: 'refuse'` (D2 as amended): this set is diffed + // against the grants the rule holds, so a failed member read answering + // "nobody" would revoke every member's grant on an outage. The refusal + // throws before any diff, and each recompute's caller keeps the rule's + // grants and warns (#29362), as for a team or position recipient. const queues = this.ownershipQueues?.(); if (!queues) return []; - return queues.expandMembers(rule.recipient_id, rule.organization_id ?? null); + return queues.expandMembers(rule.recipient_id, rule.organization_id ?? null, { onFailedRead: 'refuse' }); } return []; }