diff --git a/.changeset/spotty-pans-visit.md b/.changeset/spotty-pans-visit.md new file mode 100644 index 0000000000..d68ac4a2e0 --- /dev/null +++ b/.changeset/spotty-pans-visit.md @@ -0,0 +1,14 @@ +--- +'@objectstack/plugin-sharing': patch +--- + +Make package-seeded sharing rules visible and addressable by name to org-scoped admins + +Sharing rules seeded from an app or package are defined under the system context, so they are stored with `organization_id = null` (platform-global). `SharingRuleService.listRules` and the by-name fallback of `getRule` scoped their reads with a strict `organization_id = ` equality, which such a row can never satisfy. An authenticated org-scoped admin therefore saw `GET /api/v1/sharing/rules` return an empty list over a table of active seeded rules, and by-name `GET` and `evaluate` answered 404 `RULE_NOT_FOUND`; only the by-id branch, which was never org-scoped, still worked. + +Both admin reads now match "this organization OR platform-global", mirroring how enforcement has always read these rows under the system context. Consequences worth knowing: + +- Seeded rules now appear in the admin rule list and can be fetched, evaluated and deleted by name. An org admin could already do all three **by row id** — the by-id branch carries no org filter — so this adds an address form and discoverability, not a new authority. Deleting a package-seeded rule remains reversible: the next boot reseeds it. +- Rules belonging to a **different** organization remain invisible and unresolvable by name; only rows belonging to no organization at all become visible. +- `defineRule` is deliberately **not** widened. Its existence lookup decides upsert-vs-insert, so widening it would let one organization's admin rewrite a row every other organization reads. A same-named create still produces a row stamped with the caller's own organization, and by-name lookups prefer that row over the platform-global one. +- Callers passing a context with no organization (boot seeding, rule hooks, backfills, the boot reconcile) are unaffected — that path was already unfiltered and is unchanged. diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index 3a3e2d19a8..2f280894d8 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -221,6 +221,41 @@ export class SharingRuleService implements ISharingRuleService { return rowFromRule(newRow); } + /** + * [#7676] Tenant scope for a sharing-rule ADMIN read: "this org ∪ + * platform-global". + * + * `organization_id = null` on `sys_sharing_rule` means "owned by no + * organization" — a row written by the package/app seeder + * (`bootstrapDeclaredSharingRules`, which defines under `SYSTEM_CTX` and so + * stamps `organization_id: null`) before any org id exists. A strict + * `organization_id = ` equality made every such row invisible to + * the admin API while enforcement kept reading them under `SYSTEM_CTX`: on a + * stock boot `GET /api/v1/sharing/rules` answered `{data: []}` over four + * active seeded rules, by-name GET and evaluate 404'd `RULE_NOT_FOUND`, and + * only the org-unfiltered by-id branch of {@link getRule} still worked. Rules + * that grant access but cannot be listed, inspected or deactivated are the + * worst half of both properties. + * + * Widening the READ leaks nothing across tenants: another org's row still + * fails the match, and a null-org row is platform-global by construction — + * every org already receives the grants it materialises. This is the same + * predicate, for the same reason, that `sys_business_unit` approver expansion + * settled on in #3807 and that `sys_metadata`'s pending-draft listing uses. + * + * ⚠️ It is deliberately NOT applied to {@link defineRule}'s existence lookup. + * That lookup decides UPSERT-or-insert, so widening it would let one org's + * admin overwrite the label, criteria, recipient and access level of a row + * every OTHER org reads — a cross-tenant WRITE, which is a different act from + * a cross-tenant read of a platform-global row. A same-named POST therefore + * still creates an org-stamped row of the tenant's own, and + * {@link findRuleRowByName} prefers it. + */ + private adminOrgScope(where: Record, orgId: string | null | undefined): Record { + if (!orgId) return where; + return { ...where, $or: [{ organization_id: orgId }, { organization_id: null }] }; + } + async listRules( filter: { object?: string; activeOnly?: boolean }, context: ExecutionContext, @@ -231,9 +266,8 @@ export class SharingRuleService implements ISharingRuleService { if (filter.activeOnly) where.active = true; // `organizationId` is not on the envelope — see defineRule(). const orgId = (context as any)?.organizationId ?? context?.tenantId; - if (orgId) where.organization_id = orgId; const rows = await this.engine.find('sys_sharing_rule', { - where, + where: this.adminOrgScope(where, orgId), orderBy: [{ field: 'name', order: 'asc' }], limit: 1000, context: SYSTEM_CTX, @@ -252,15 +286,34 @@ export class SharingRuleService implements ISharingRuleService { context: SYSTEM_CTX, }); if (Array.isArray(byId) && byId[0]) return rowFromRule(byId[0]); - const byName = await this.engine.find('sys_sharing_rule', { - where: orgId ? { name: idOrName, organization_id: orgId } : { name: idOrName }, - limit: 1, - context: SYSTEM_CTX, - }); - if (Array.isArray(byName) && byName[0]) return rowFromRule(byName[0]); + const byName = await this.findRuleRowByName(idOrName, orgId); + if (byName) return rowFromRule(byName); return null; } + /** + * [#7676] Resolve a rule by NAME for an admin read: this org first, the + * platform-global (`organization_id IS NULL`) row second. + * + * Two sequenced lookups rather than one `$or` with `limit: 1`, because when + * BOTH rows exist the answer must be the caller's own: a single disjunctive + * query with a row cap picks whichever row the driver happened to reach + * first, so an org that had authored its own `share_red_projects_with_execs` + * could get the platform row back on one dialect and its own on another. + * Preference is a decision, so it is written as one. + * + * No `orgId` (SYSTEM_CTX — boot seeding, hooks, backfills) keeps the + * unfiltered by-name lookup it has always had. + */ + private async findRuleRowByName(name: string, orgId: string | null | undefined): Promise { + const first = async (where: Record): Promise => { + const rows = await this.engine.find('sys_sharing_rule', { where, limit: 1, context: SYSTEM_CTX }); + return Array.isArray(rows) && rows[0] ? rows[0] : null; + }; + if (!orgId) return first({ name }); + return (await first({ name, organization_id: orgId })) ?? (await first({ name, organization_id: null })); + } + async deleteRule(idOrName: string, context: ExecutionContext): Promise { this.assertCanManageRules(context); // [ADR-0111 D6] const row = await this.getRule(idOrName, context); diff --git a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts index f438ff8590..92669d4593 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts @@ -23,8 +23,16 @@ function makeEngine() { const ensure = (n: string) => (tables[n] ??= []); function matches(row: Row, f: any): boolean { if (!f || typeof f !== 'object') return true; - if (Array.isArray(f.$or)) return f.$or.some((x: any) => matches(row, x)); - if (Array.isArray(f.$and)) return f.$and.every((x: any) => matches(row, x)); + // [#7676] A combinator is CONJOINED with its sibling field keys, never a + // short-circuit that returns before they are read. This used to + // `return f.$or.some(...)`, silently DROPPING every sibling — so + // `listRules`'s `{object_name, active, $or:[…org scope…]}` would have + // matched the whole table here while the real drivers (driver-sql's + // `applyFilterCondition`, driver-memory's mingo document) AND the two. + // A fake looser than the contract it stands in for is how a green suite + // ships a broken filter (the #4434 lesson, applied to reads). + if (Array.isArray(f.$or) && !f.$or.some((x: any) => matches(row, x))) return false; + if (Array.isArray(f.$and) && !f.$and.every((x: any) => matches(row, x))) return false; for (const [k, v] of Object.entries(f)) { if (k === '$or' || k === '$and') continue; const rv = row[k]; @@ -850,3 +858,164 @@ describe('[ADR-0111 D6] sharing-rule management gate', () => { expect(r.id).toBeTruthy(); }); }); + +// ───────────────────────────────────────────────────────────────────── +// [#7676] Package-seeded rules (`organization_id = null`) are visible and +// addressable BY NAME to an org-scoped admin. +// +// The QA run's finding: on a stock boot `GET /api/v1/sharing/rules` answered +// `{data: []}` over four active seeded rules, by-name GET/evaluate 404'd +// `RULE_NOT_FOUND`, and only the org-unfiltered by-id branch still worked — +// because the seeder defines under SYSTEM_CTX (`organization_id: null`) while +// an authenticated admin's context carries `organizationId: 'org_…'`, and a +// strict equality can never match. Enforcement was unaffected (boot reconcile +// reads under SYSTEM_CTX, no org filter), which is exactly why it stayed +// invisible: rules that grant access but cannot be listed or deactivated. +// ───────────────────────────────────────────────────────────────────── + +describe('[#7676] admin visibility of package-seeded (org-null) sharing rules', () => { + let engine: ReturnType; + let rules: SharingRuleService; + + /** An authenticated org-scoped sharing admin — the shape the REST layer builds. */ + const ORG1_ADMIN = { userId: 'admin', organizationId: 'org1', systemPermissions: ['manage_sharing'] } as any; + const SEEDED = 'share_red_projects_with_execs'; + /** + * The seeder's own context — `bootstrapDeclaredSharingRules` defines under + * plugin-sharing's `SYSTEM_CTX`, which carries NO organization, and that is + * precisely what stamps `organization_id: null` on the row. Not this file's + * older `SYS`, which is `{isSystem: true, organizationId: 'org1'}` — system + * only bypasses the ADR-0111 D6 capability gate, never the org scope. + */ + const BOOT = { isSystem: true, positions: [], permissions: [] } as any; + + beforeEach(async () => { + engine = makeEngine(); + engine._tables.project = [ + { id: 'p_red', status: 'red', owner_id: 'someone' }, + { id: 'p_green', status: 'green', owner_id: 'someone' }, + ]; + rules = new SharingRuleService({ engine: engine as any, sharing: new SharingService({ engine: engine as any }) }); + + // Package seed — defined under SYSTEM_CTX exactly as + // `bootstrapDeclaredSharingRules` does, so `organization_id` lands null. + await rules.defineRule({ + name: SEEDED, label: 'Red projects → execs', object: 'project', + criteria: { status: 'red' }, recipientType: 'user', recipientId: 'exec', + managedBy: 'package', + } as any, BOOT); + // A rule belonging to a DIFFERENT organization — the isolation pin. + await rules.defineRule({ + name: 'other_org_rule', label: 'Other org', object: 'project', + criteria: { status: 'green' }, recipientType: 'user', recipientId: 'mallory', + } as any, { userId: 'other', organizationId: 'org2', systemPermissions: ['manage_sharing'] } as any); + // An API-created rule stamped with THIS org — must keep working as today. + await rules.defineRule({ + name: 'org1_rule', label: 'Org1 own', object: 'project', + criteria: { status: 'red' }, recipientType: 'user', recipientId: 'alice', + } as any, ORG1_ADMIN); + }); + + it('the fixture really does store a null organization_id on the seeded row', () => { + const seeded = engine._tables.sys_sharing_rule.find((r) => r.name === SEEDED); + expect(seeded?.organization_id).toBeNull(); + expect(seeded?.managed_by).toBe('package'); + // …and the org-stamped rows really are stamped, or the pins below prove nothing. + expect(engine._tables.sys_sharing_rule.find((r) => r.name === 'org1_rule')?.organization_id).toBe('org1'); + expect(engine._tables.sys_sharing_rule.find((r) => r.name === 'other_org_rule')?.organization_id).toBe('org2'); + }); + + // ── visibility (the defect) ────────────────────────────────────────── + + it('listRules shows the seeded rule to an org-scoped admin', async () => { + const names = (await rules.listRules({}, ORG1_ADMIN)).map((r) => r.name); + expect(names).toContain(SEEDED); + // Exact set — "this org ∪ platform-global", and nothing else. Kept HERE + // rather than on the isolation pins below so that each test has ONE + // predicted direction under the ablation: this one flips red, they do not. + expect(names.sort()).toEqual([SEEDED, 'org1_rule'].sort()); + }); + + it('getRule resolves the seeded rule BY NAME for an org-scoped admin', async () => { + const row = await rules.getRule(SEEDED, ORG1_ADMIN); + expect(row?.name).toBe(SEEDED); + expect(row?.organization_id).toBeNull(); + }); + + it('evaluateRule works BY NAME for the seeded rule (no RULE_NOT_FOUND)', async () => { + const res = await rules.evaluateRule(SEEDED, ORG1_ADMIN); + expect(res.matchedRecords).toBe(1); + expect(res.grantsCreated).toBe(1); + // The control the QA run used: by-id already worked, and must still agree. + const byId = await rules.getRule(SEEDED, ORG1_ADMIN); + expect((await rules.evaluateRule(byId!.id, ORG1_ADMIN)).ruleId).toBe(byId!.id); + }); + + // ── isolation pins (must stay green under the ablation) ────────────── + + it('another organization’s rule stays invisible — list', async () => { + const names = (await rules.listRules({}, ORG1_ADMIN)).map((r) => r.name); + expect(names).not.toContain('other_org_rule'); + // Positive half, so this cannot pass by listing nothing at all — and it is + // the org's OWN row deliberately, which survives the ablation. + expect(names).toContain('org1_rule'); + }); + + it('another organization’s rule stays unresolvable — by name', async () => { + expect(await rules.getRule('other_org_rule', ORG1_ADMIN)).toBeNull(); + await expect(rules.evaluateRule('other_org_rule', ORG1_ADMIN)).rejects.toThrow(/RULE_NOT_FOUND/); + }); + + it('the org scope is CONJOINED with the object/activeOnly filters, not substituted for them', async () => { + await rules.defineRule({ + name: 'seeded_other_object', label: 'Other object', object: 'account', + criteria: { tier: 'gold' }, recipientType: 'user', recipientId: 'exec', + managedBy: 'package', + } as any, BOOT); + await rules.defineRule({ + name: 'seeded_inactive', label: 'Inactive seed', object: 'project', + criteria: { status: 'red' }, recipientType: 'user', recipientId: 'exec', + managedBy: 'package', active: false, + } as any, BOOT); + + // Both halves assert on the org's OWN row for the positive side, so this + // test stays GREEN under the ablation and goes red only for its own cause: + // an org scope that SUBSTITUTED for the object/activeOnly predicates + // instead of being conjoined with them (the exact way a top-level `$or` + // fails when a filter evaluator short-circuits on the combinator). + const projects = (await rules.listRules({ object: 'project' }, ORG1_ADMIN)).map((r) => r.name); + expect(projects).not.toContain('seeded_other_object'); + expect(projects).toContain('org1_rule'); + const active = (await rules.listRules({ object: 'project', activeOnly: true }, ORG1_ADMIN)).map((r) => r.name); + expect(active).not.toContain('seeded_inactive'); + expect(active).toContain('org1_rule'); + }); + + // ── unchanged behaviour ────────────────────────────────────────────── + + it('an API-created org-stamped rule is still listed and gettable by name', async () => { + expect((await rules.getRule('org1_rule', ORG1_ADMIN))?.organization_id).toBe('org1'); + expect((await rules.listRules({}, ORG1_ADMIN)).map((r) => r.name)).toContain('org1_rule'); + }); + + it('a no-org (SYSTEM_CTX / boot) context still sees every rule, unfiltered', async () => { + const names = (await rules.listRules({}, BOOT)).map((r) => r.name).sort(); + expect(names).toEqual([SEEDED, 'org1_rule', 'other_org_rule'].sort()); + expect((await rules.getRule('other_org_rule', BOOT))?.organization_id).toBe('org2'); + }); + + it('when both a platform-global and an own-org row share a name, by-name resolves the OWN row', async () => { + // `defineRule` is deliberately NOT widened, so a same-named POST from an + // org admin creates its own row instead of overwriting the shared seed. + const own = await rules.defineRule({ + name: SEEDED, label: 'Org1 override', object: 'project', + criteria: { status: 'red' }, recipientType: 'user', recipientId: 'alice', + } as any, ORG1_ADMIN); + expect(engine._tables.sys_sharing_rule.filter((r) => r.name === SEEDED)).toHaveLength(2); + expect(engine._tables.sys_sharing_rule.find((r) => r.organization_id === null)?.label) + .toBe('Red projects → execs'); // the seed row is untouched + const resolved = await rules.getRule(SEEDED, ORG1_ADMIN); + expect(resolved?.id).toBe(own.id); + expect(resolved?.organization_id).toBe('org1'); + }); +});