fix(plugin-sharing): make package-seeded (org-null) sharing rules visible and addressable by name for org-scoped admins - #7760
Conversation
…ible and addressable by name
Rules seeded from an app or package are defined under the system context, so
they are stored with `organization_id = null` (platform-global). `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 org-scoped admin saw `GET /api/v1/sharing/rules` answer `{"data":[]}` over a
table of active seeded rules, and by-name GET/evaluate answered 404
`RULE_NOT_FOUND`. Only the by-id branch, which carries no org filter, worked.
Enforcement was unaffected — the boot reconcile also reads under the system
context — which is exactly why this stayed invisible.
Both admin reads now match "this organization OR platform-global", the same
predicate `sys_business_unit` approver expansion settled on in #3807 and the
one `sys_metadata`'s pending-draft listing uses. Another organization's row
still fails the match; only rows belonging to no organization 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 cross-tenant WRITE, a different act from
a cross-tenant read of a platform-global row. A same-named create therefore
still produces a row stamped with the caller's own organization, and
`findRuleRowByName` prefers that row over the platform-global one via two
sequenced lookups rather than one `$or` with `limit: 1`, so the preference is a
decision rather than whichever row a dialect happened to reach first.
The test fake engine's filter matcher short-circuited on `$or` and DROPPED its
sibling field keys, so `listRules`'s `{object_name, active, $or:[…]}` would have
matched the whole table there while driver-sql and driver-memory conjoin the
two. A fake looser than the contract it stands in for is how a green suite
ships a broken filter, so it now conjoins.
Fixes #7676
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BVc1ekPpi6yaWywAUhfzfd
…ring-rules-org-null
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 7 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also reference the affected code. These are read-only:
|
Fixes #7676
The defect
Sharing rules seeded from an app or package are defined by
bootstrapDeclaredSharingRulesunder plugin-sharing'sSYSTEM_CTX— a context carrying no organization — so every seeded row lands withorganization_id = null(platform-global).SharingRuleService.listRulesand the by-name fallback ofgetRulescoped their reads with a strictorganization_id = <caller org>equality, which such a row can never satisfy.On a stock boot, as an org-scoped admin:
GET /api/v1/sharing/rules{"data":[]}over 4 active seeded rulesGET /api/v1/sharing/rules/<seeded name>RULE_NOT_FOUNDPOST /api/v1/sharing/rules/<seeded name>/evaluatePOST /api/v1/sharing/rules/<row id>/evaluate(control)Enforcement was never affected — the boot reconcile also reads under
SYSTEM_CTX, no org filter — which is exactly why this stayed invisible. The result is the worst half of both properties: rules that grant access org-wide while being impossible to list, inspect or deactivate.Premise re-verified on
origin/mainbefore implementing: both sites are present as reported,listRulesat theif (orgId) where.organization_id = orgIdline andgetRule'sorgId ? {name, organization_id: orgId} : {name}fallback.The change
Both admin reads now match "this organization OR platform-global" — the same predicate
sys_business_unitapprover expansion settled on in #3807 (businessUnitOrgScope) and the onesys_metadata's pending-draft listing uses. Another organization's row still fails the match; only rows belonging to no organization at all become visible.Two details that are decisions rather than mechanics:
defineRuleis deliberately NOT widened. Its existence lookup decides upsert-vs-insert, so widening it would let one organization's admin rewrite the label, criteria, recipient and access level of a row every other organization reads — a cross-tenant WRITE, which is a different act from a cross-tenant READ of a platform-global row. A same-named create therefore still produces a row stamped with the caller's own organization.$orwithlimit: 1. A single disjunctive query with a row cap returns whichever row the driver reached first, so an org that had authored its ownshare_red_projects_with_execscould get the platform row back on one dialect and its own on another. Preference is a decision, so it is written as one.Tenant-isolation audit (pre-check before widening)
Every caller of
listRules/getRule, and whether it relies on the strict equality for isolation:rest-server.tsGET/sharing/rules→listRulesrest-server.tsGET/sharing/rules/:idOrName→getRulerest-server.tsDELETE/sharing/rules/:idOrName→deleteRule→getRulerest-server.tsPOST/sharing/rules/:idOrName/evaluate→evaluateRule→getRulerest-server.tsPOST/sharing/rules→defineRulesharing-plugin.ts×5 (boot reconcile, backfill, hooks){isSystem: true}, no orgsharing-rule-service.tsevaluateAllForRecord→listRules(context)SYSTEM_CTXfromrule-hooks.tssharing-rule-service.tsevaluateAllRulesForObject→listRules(SYSTEM_CTX)bootstrap-declared-sharing-rules.ts→defineRule(SYSTEM_CTX)Does read-widening become write/delete-widening on package-seeded rows?
deleteRuleandevaluateRuleboth resolve throughgetRule, so yes, they gain the by-name address. That is kept because it is symmetric with what by-id already allows:getRule's by-id branch carries no org filter at all, so an org admin could already delete and evaluate a null-org seeded rule by row id today — the issue's own control experiment is a successful by-id evaluate. This change adds an address form and discoverability, not a new class of authority. It is also consistent with the seed machinery's stated intent (#2909: admins may deactivate/customize seeded rules and that must survive redeploys), and a deleted seeded rule is reseeded on the next boot.No cross-tenant exposure is created. Rules belonging to a different organization remain excluded from both widened reads.
One pre-existing gap found during the audit and filed separately rather than fixed here:
getRule's by-id branch is org-unfiltered, so an org admin holding another organization'ssrule_…id can read, evaluate and delete that organization's rule. It predates this change, is not made worse by it, and narrowing it is a behaviour change deserving its own PR.Tests
packages/plugins/plugin-sharing/src/sharing-rule.test.ts— a[#7676]block with an org-scoped admin ({userId, organizationId: 'org1', systemPermissions: ['manage_sharing']}), a package seed defined under an org-less boot context, an org1-owned rule and an org2-owned rule:organization_id: nullon the seed and real org stamps on the other two, so the pins below cannot pass for the wrong reason.listRules, resolves by name, evaluates by name (and agrees with the by-id control).object/activeOnly, not substituted for them.SYSTEM_CTX/boot) context still sees everything unfiltered; when both a platform-global and an own-org row share a name, by-name resolves the own row.The test fake engine's filter matcher short-circuited on
$orand dropped its sibling field keys, solistRules's{object_name, active, $or:[…]}would have matched the whole table there while driver-sql'sapplyFilterConditionand driver-memory's mingo document conjoin the two. It now conjoins — a fake looser than the contract it stands in for is how a green suite ships a broken filter. No existing test depended on the loose behaviour.Reverse verification (ablation)
Predicted before running: the three visibility tests flip red, every isolation and unchanged-behaviour pin stays green. Reverting only
sharing-rule-service.tsto its pre-fix state and keeping the tests, on the post-merge tree:Exactly the predicted split — the two isolation pins, the conjunction pin, the org-stamped pin, the no-org pin and the both-rows-share-a-name pin all stayed green, so they are pinning isolation rather than riding on the visibility change.
One thing this PR does NOT ship, deliberately
An HTTP-layer regression test in
packages/qa/dogfoodwas written and then removed after it proved it could not discriminate.bootStack's admin carries no organization — a rule created through the API in that harness comes backorganization_id: null— so the org-scoped filter path is never entered and the test was green on the unfixed code for the wrong reason. An org-scoped admin needsmultiTenant: true, which requires the cloud-private@objectstack/organizationspackage and is not reachable in the open core. Shipping it would have been a phantom check that reads as coverage. This is filed as a coverage-harness finding, and it is the direct reason this defect escaped both the plugin's 428-test suite and the 579-test dogfood suite and reached a QA run.Generated by Claude Code