fix(plugin-sharing): make package-seeded (org-null) sharing rules visible and addressable by name for org-scoped admins

.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 = <caller org>` 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.

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 = <request org>` 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<string, unknown>, orgId: string | null | undefined): Record<string, unknown> {
+ 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<any | null> {
+ const first = async (where: Record<string, unknown>): Promise<any | null> => {
+ 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<void> {
  this.assertCanManageRules(context); // [ADR-0111 D6]
  const row = await this.getRule(idOrName, context);

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<typeof makeEngine>;
+ 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');
+ });
+});