Skip to content

fix(platform-objects,plugin-security,driver-sql): scope sys_user_preference and sys_capability uniqueness per organization (#8323) - #8461

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-8323-tenant-scoped-unique-indexes
Aug 13, 2026
Merged

fix(platform-objects,plugin-security,driver-sql): scope sys_user_preference and sys_capability uniqueness per organization (#8323)#8461
os-zhuang merged 3 commits into
mainfrom
claude/issue-8323-tenant-scoped-unique-indexes

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#8323

Implements option 2 only, per the maintainer ruling of 2026-08-13 10:05Z. target:v17, security — release blocker.

What was wrong

sys_user_preference and sys_capability both declared their uniqueness as a table-level index with bare unique: true. At the declared-index level that is the positional spelling of 'global' — the listed columns verbatim — so on a tenant-scoped object it materialized an installation-wide unique index:

objectdeclaredmaterialized (before)
sys_user_preference{ fields: ['user_id','key'], unique: true }uniq_sys_user_preference_user_id_key (user_id, key)
sys_capability{ fields: ['name'], unique: true }uniq_sys_capability_name (name)

Both now say unique: 'organization' (ADR-0120 D1), materializing (COALESCE(organization_id,'__global__'), ...).

Why #4986 did not cover this — measured, and it is NOT a regression

The two unique spellings mean opposite things at the two levels, deliberately:

#4986 landed the 'organization' token on the declared-index path; it requires the explicit spelling. packages/lint/src/data-model-rules.ts names this exact situation "the #4986 trap" and ships unique/unscoped-declared-index (warning in 17.x) for it — these two objects were its two instances in the platform's own metadata. Pinned in sql-driver-declared-index-organization-respelling.test.ts under "the two unique spellings diverge by design", including a positive pin that field-level bare true still scopes per organization.

Nothing about bare-spelling semantics is touched — that stays on #5082, and the publish-time authoring advisory is out of scope here (#8379 remains open and unaffected).

Migration staging, and why schema-drift.ts is in this PR

Respelling a declared index changes its generated name. On a deployed database that read as two unrelated findings: the composite missing (create_index, safe) and the old global index orphaned (drop_index, destructive). An operator applying only the safe half keeps the global index — i.e. keeps the defect — while the plan reads as applied.

Measured, by reverting only schema-drift.ts and re-running: after a non-destructive apply, the cross-organization insert still answered 409 (expected 409 to be 201). The object-declaration change alone does not fix a deployed install.

So the respelling now routes through the same replace_unique_index retirement the field-level migration has used since #3728 — reused, not re-rolled. LegacyUniqueReplacement gains legacyColumns so the matcher can recognise a multi-column legacy shape (with plainness guards: an index carrying an expression key part, a NULL-safe part or a WHERE predicate is not the verbatim global shape). Result on a deployed database: one finding for the table, categorised safe, CREATE before DROP, legacy index dropped only once the replacement is confirmed present. No --allow-destructive required, and the constraint is never unenforced at any point — initObjects materializes the composite additively at boot, and the plan owns only the retirement.

Reverse verification — both halves, predicted before running

directionpredictedactual
cross-org duplicate, after201201
same-org duplicate, after (anti-vacuity)409 UNIQUE_VIOLATION409
cross-org duplicate, before409409
revert schema-drift.ts5 red: 4 migration + 1 guard; behavioural tests stay greenexactly those 5 red, 21 green
revert both object declarationspreference pin 4 red / 2 green; capability pin 3 red / 1 greenexact match on both

No prediction was contradicted, so nothing was retro-fitted.

Both fixtures — pre-fix and post-fix — are kept permanently in the suite, so the contrast is a standing assertion rather than a one-off measurement. Rejection cases assert the envelope (isUniqueViolationError is what rest-server.ts maps to 409 + code: UNIQUE_VIOLATION), never a bare toThrow().

§3 confirmed end to end: zhangsan in two organizations now holds an independent ui.recent in each; the same key twice in one organization is still refused; a different user in the same organization is unaffected.

Platform seed unaffected: platform capability rows carry no organization and the key part is NULL-safe (ADR-0120 D3), so they stay unique among themselves and bootstrapSystemCapabilities' upsert-by-name still resolves. An organization may now define its own row of the same name — the ADR-0066 "admins EXTEND the registry" case.

File surface

filechange
packages/platform-objects/src/identity/sys-user-preference.object.ts(user_id, key)unique: 'organization'
packages/plugins/plugin-security/src/objects/sys-capability.object.ts(name)unique: 'organization' (cross-lane, domain:identity)
packages/drivers/driver-sql/src/schema-drift.tsdeclared-index arm of legacyUniqueReplacements; legacyColumns on the matcher
packages/drivers/driver-sql/src/sql-driver-declared-index-organization-respelling.test.tsnew — 26 tests
packages/platform-objects/src/identity/sys-user-preference.organization-unique.test.tsnew — declaration pin (6)
packages/plugins/plugin-security/src/objects/sys-capability.organization-unique.test.tsnew — declaration pin (4)
packages/plugins/plugin-security/src/objects/rbac-objects.test.tsrespelled a pin that asserted toBe(true)
packages/drivers/driver-sql/src/sql-driver-index-drift.test.ts, ...-index-introspection-failure.test.tslegacyColumns on hand-built literals
.changeset/tenant-scoped-platform-object-uniques.mdnew

packages/metadata-protocol/src/protocol.ts is not touched.

Verification

  • driver-sql 1446 passed / 52 skipped; platform-objects 357 passed; plugin-security 1037 passed. All three typecheck clean.
  • Full workspace build: 71/71 successful.
  • Gates re-derived with scripts/pm/dispatch-gates.mjs against the actual changed paths and run locally — all green: nul-bytes, query-options-erasure, type-check-coverage, driver-conformance, cross-package-test-inputs, test-source-alias, type-source-resolution, tenant-chokepoint, adr-anchors, changeset-gate-self-tests, objectui-changeset, docs-audit-scope, empty-changeset, check-changeset-no-major, check-dev-prereqs.
  • check:query-options-erasure initially flagged the test surface growing 240 → 241. Fixed at the cause (typed the query instead of as any); the ledger is back at 240, not grown.
  • origin/main merged before pushing; everything above re-run after the merge.

Generated by Claude Code

…erence and sys_capability uniqueness per organization
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
…eclarations; changeset
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012WMpuAfA2KSdDjGF6tm1bH
@vercel

vercelBot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 13, 2026 2:15pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/driver-sql, @objectstack/platform-objects, @objectstack/plugin-security.

19 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via @objectstack/driver-sql)
  • content/docs/deployment/cli.mdx(via @objectstack/plugin-security)
  • content/docs/getting-started/glossary.mdx(via @objectstack/driver-sql)
  • content/docs/kernel/runtime-services/sharing-service.mdx(via @objectstack/plugin-security)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/driver-sql, @objectstack/plugin-security)
  • content/docs/permissions/access-recipes.mdx(via packages/plugins/plugin-security)
  • content/docs/permissions/authorization.mdx(via @objectstack/plugin-security)
  • content/docs/permissions/explain.mdx(via @objectstack/plugin-security)
  • content/docs/permissions/permissions-matrix.mdx(via packages/plugins/plugin-security)
  • content/docs/permissions/sharing-rules.mdx(via @objectstack/plugin-security)
  • content/docs/plugins/anatomy.mdx(via @objectstack/driver-sql)
  • content/docs/plugins/index.mdx(via @objectstack/plugin-security)
  • content/docs/plugins/packages.mdx(via @objectstack/driver-sql, @objectstack/platform-objects, @objectstack/plugin-security)
  • content/docs/protocol/kernel/index.mdx(via @objectstack/driver-sql)
  • content/docs/protocol/kernel/lifecycle.mdx(via @objectstack/driver-sql)
  • content/docs/protocol/objectql/query-syntax.mdx(via @objectstack/driver-sql)
  • content/docs/ui/audience-based-interfaces.mdx(via packages/plugins/plugin-security)
  • content/docs/ui/dashboards.mdx(via @objectstack/plugin-security)
  • content/docs/ui/setup-app.mdx(via @objectstack/platform-objects)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/driver-sql, @objectstack/plugin-security)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

Docs check — answer: no published doc states or implies installation-wide uniqueness. Nothing to fix, no doc edits in this PR.

Checked the four candidates plus the whole content/docs/ tree. content/docs/releases/** not touched either way.

sys_capability

Every published mention describes the registry, its provenance and its seeding — none makes a uniqueness claim:

locationsaysnow false?
permissions/authorization.mdx:179bootstrapDeclaredCapabilities seeds with managed_by:'package' + package_id, "idempotent, re-seeded on upgrade, never clobbers admin-authored rows"no — still exact. Platform/package rows carry no organization, and the NULL-safe key part keeps them unique among themselves, so upsert-by-name resolves unchanged
permissions/authorization.mdx:393capabilities seeded as first-class records from PLATFORM_CAPABILITIESno
permissions/permission-sets.mdx:138"Capabilities are first-class sys_capability records — named privileges"no — names them, claims nothing about uniqueness
references/security/misc.mdx:26name = "Stable capability key referenced by systemPermissions / requiredPermissions"no — "stable", not "unique". Generated from the field's describe()

The four files you named were the right places to look, and three of them (permissions-matrix.mdx, access-recipes.mdx, explain.mdx) contain no uniqueness language at all. The only exactly one in authorization.mdx is line 370, about the authz conformance matrix's state enum ("every authorization primitive sits in exactly one honest state") — unrelated.

Worth noting the reference table already documents scope as "platform = a platform-wide power; org = scoped to an organization" — the published model already carries the per-organization notion this PR makes physical, so the change moves the index toward the documented semantics rather than away.

sys_user_preference

Nothing documents (user_id, key) as installation-unique. Only two live mentions: data-modeling/objects.mdx:277,284, both about the managedBy: 'system-data' bucket, and the code excerpt there elides indexes with // …. (The other two hits are releases/v15.mdx and v16.mdx — untouched.)

One thing the sweep did turn up — filed, not fixed

content/docs/references/identity/position.mdx:64 documents sys_position.name as "Unique position name". That is the one place a strong uniqueness claim is published — and sys_position carries { fields: ['name'], unique: true } with no tenancy opt-out and managedBy: 'config', i.e. structurally identical to the sys_capability instance this PR fixes.

Filed as #8468 rather than folded in: it is outside the two objects the ruling named, and — unlike capabilities, where ADR-0066 D1's "admins EXTEND the registry" settles intent — I found no equivalent statement for positions, so whether installation-wide is wrong or merely unstated is a real question. If it is wrong the fix is one word plus the migration this PR already generalized to declared indexes; if it is right, the correct fix is the opposite (unique: 'global' explicitly, and the describe() stays). I did not want to guess on a security-class object.

I also swept the remaining platform objects: most bare declared uniques are legitimately installation-wide (sys_session.token, sys_api_key.key, sys_oauth_*.token, sys_device_code.*) and several already hand-write the organization composite (sys_team, sys_business_unit, sys_member — the ADR-0120 S6 spelling, valid indefinitely). sys_position was the only one that patterned like the fixed pair.

Standing by — not marking ready, not enabling auto-merge, not merging.


Generated by Claude Code


Generated by Claude Code

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

Reachability measured — ⛔ outcome 3, stopping as instructed

Two separate answers. The thing I originally flagged is not reachable. Measuring it surfaced a different, real consequence of the same root cause, which I have not fixed and have not guarded against. PR unchanged at df32d016fa; working tree clean.

1. Can bootstrapSystemCapabilities run with an organization in context? No — structurally impossible.

#call sitecarries a tenant context?
1security-plugin.ts:2538, the sole production call, inside runBootstrapno
2ctx.hook('kernel:ready', runBootstrap) (:2563) — bootno
3void runBootstrap() (:2565) — fallback when the kernel exposes no hook (test stubs)no
4ql.registerMiddleware post-sys_user create/insert (:2581-2590) — re-runs bootstrap after a signup, which can occur inside a tenant-scoped requestthe trigger can; the seeder cannot

Trigger 4 is the one worth the check, and it is closed by three independent facts:

  • The seeder has no context channel.SeedOptions is { logger?, materializedCapabilityNames? }. There is no parameter through which an organization could arrive.
  • It builds its own context.const SYSTEM_CTX = { isSystem: true } is a module-level constant with no tenantId, passed explicitly on all three access paths (ql.find / ql.insert / ql.update). The triggering opCtx is never forwarded — the middleware calls runBootstrap() with no arguments, and runBootstrap closes over the plugin-level ql, not a per-request engine.
  • No ambient tenant to leak. The only AsyncLocalStorage in the engine is txStore (transactions). There is no async-local tenant store.

The stamp itself fills from the caller's active org, and this caller has none. So the org-local duplicate I flagged cannot occur. That worry is dead — and it would have been dead before this PR too.

2. What the measurement did find — the constraint was masking something else

tryFind(ql, 'sys_capability', { name }, 1) runs under isSystem, i.e. across organizations. Before this PR the global unique made an org-authored row named manage_users impossible (409). After it, that row is permitted — my own test asserts it, per ADR-0066 "admins EXTEND the registry". So the seeder's find-by-name can now return an organization's row where it previously could only ever return the platform's.

Measured against the seeder's own in-memory ql harness (which models isSystem faithfully — it filters on where only, ignoring organization). Direction predicted before running; all three matched:

caseresult
DERIVED name (approve_invoice) + org rowsafeskippedAuthored: 1, org label preserved. The derivedNames.has(name) && row.managed_by !== 'platform' guard fires
CURATED name, platform row seeded first, org row added after✅ safe in the harness — platform row refreshed, org row untouched, 2 rows coexist
CURATED name (manage_users), org row present, platform row not yetplatformRowExists: false — the platform's own row is never seeded, and the org row's ORG CUSTOM LABEL is overwritten with Manage Users

Two harms, the second worse than the first:

  1. An organization's authored capability row is silently overwritten with the platform's copy at boot.
  2. A single tenant's row suppresses the platform's own definition installation-wide — the curated row is never created, in any bucket.

⚠️The "safe" middle row is weaker than it looks. It holds only because the fake returns rows in insertion order. The real query is find(..., { where: { name }, limit: 1 }) with no ORDER BY, and SQL guarantees no ordering there — so once two rows share a name, which one gets reconciled is arbitrary on a real database and may flip between boots.

Scope, stated honestly: I measured seeding behaviour only. I did not measure whether a missing platform capability row changes any authorization decision — capability rows are definitions and grants live in permission sets, so I have no evidence of privilege impact, and I am not claiming any. Only curated names are exposed (~8 of them); derived names are guarded. sys_user_preference is unaffected — nothing seeds preferences by name across organizations.

3. What I did not do

No fix, no guard, no PR widening — per your instruction, and because an unmeasured guard here would be worse than the measured absence. I have also not filed a card, since you said the sequencing is yours; say the word and I will file it with this measurement attached.

For sequencing: reverting is strictly worse — the global unique this PR removes is the release-blocking cross-tenant oracle, and this residual is a narrower, non-security correctness issue that requires an admin to author a capability whose name exactly matches a curated platform one. My read is that it is a follow-up, not a blocker, but that is your call.

Standing by. Not marking ready, not enabling auto-merge, not merging.


Generated by Claude Code


Generated by Claude Code

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

Residual filed as #8470 (bug, unassigned, no needs-user-decision — PM-sequenced; this PR is not held).

The card carries the three measured rows with row 2 marked safe in the harness only (the real find(..., limit: 1) has no ORDER BY, so once two rows share a name the choice is arbitrary on a real database and can flip between boots — row 2 is row 3 with an unobserved coin flip), platformRowExists: false as the concrete failure, the exact 8 curated names, and my scope statement verbatim: seeding measured, authorization NOT measured, with "does a never-seeded curated capability break any downstream assumption?" left as an explicit open question rather than answered by my reasoning.

It also states plainly that #8461unmasks rather than causes it — the nondeterministic limit: 1 predates this PR; the global unique merely made the two-row state unreachable — and that ⛔ restoring the global unique is not the remedy, since that index is the release-blocking cross-tenant oracle itself.

No change to this PR. df32d016fa remains what is under review; still draft, auto-merge not enabled.


Generated by Claude Code


Generated by Claude Code

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

ACCEPT — PM review, domain:metadata seat. target:v17 release blocker.

Green at df32d016: all 25 checks, each job's own conclusion verified individually. Path-fork check clean — 10 files, none under docs/adr/**, .claude/skills/** or skills/**, and the surface matches the PR body's declared table exactly, including the cross-lane plugin-security file.

I read schema-drift.ts rather than the description of it. That is the file carrying the risk — a migration over deployed data on a security fix — and the review bar it had to clear was not "does it work" but "can an operator end up in a state where this reads as applied and is not".

The finding that makes this PR more than two declaration edits

Respelling a declared index changes its generated name, so on a deployed database it reads as two unrelated findings: composite missing (create_index, safe) and old global index orphaned (drop_index, destructive, opt-in). An operator applying only the safe half keeps the global index — which is the defect — while the plan reads as applied. Measured, not reasoned: reverting only schema-drift.ts and re-running, a non-destructive apply still answered 409 cross-organization (expected 409 to be 201).

Changing the two declarations alone — exactly what the ruling's letter asked for — would have passed every behavioural test on a fresh database and left every deployed installation still enumerable. On a release blocker, that is the worst available outcome, and it was one commit away.

Why the retirement route is safe, verified in the code

  • It reuses the same replace_unique_index op the field-level retirement has used since unique 索引迁移在启动时静默执行 DDL,os migrate plan 看不到 —— 运维无预检手段 #3728 — reused, not re-rolled, as instructed.
  • One finding per table, categorised safe, CREATE before DROP, legacy dropped only once the replacement is confirmed present. So uniqueness is never unenforced at any point — the property without which this would not be acceptable on a security fix at all.
  • The relaxation argument is sound: any two rows colliding on (tenant, …listed) already collided on (…listed), so the create cannot fail on existing data.

Two guards I did not specify and would not have thought to:

  1. An explicitly named index is skipped — it keeps its name across the respelling, so there is no second name to retire, and emitting a replacement would propose dropping the very index recreate_index is rebuilding.
  2. legacyName === replacement.name is skipped — the ADR-0120 S6 hand-written composite already lists the tenant column, so nothing was superseded. That is exactly what protects sys_team, sys_business_unit and sys_member, which the same session's sweep independently identified as already hand-writing the composite. The guard and the sweep agree, which is a good sign that both are right.

The matcher tightening is correspondingly careful: exact column list in key order, plus plainness guards rejecting an expression key part, a NULL-safe part or a WHERE predicate. The comment names why the multi-column arm needs it — (user_id, key) and (user_id, tenant) share a leading column and only one is the index being replaced.

The bar this card was dispatched under

The residual, and why it does not hold this PR

Measuring a flagged worry killed it (bootstrapSystemCapabilities cannot carry a tenant context — three-way structural closure, not an absence of evidence) and surfaced a real one, now filed as #8470: the platform seeder's find(…, limit: 1) has no ORDER BY, so once an organization authors a capability with a curated name the choice is arbitrary and the platform's own row may never be seeded.

Ruled: that does not block this. The global unique this PR removes is the release-blocking cross-tenant oracle; holding the fix to avoid a strictly lesser residual keeps the worse hole open. The residual needs an admin to author one of 8 exact curated names; the oracle needed nothing but a request. ⛔ And restoring the global unique is not the remedy — #8470 says so explicitly, because that index is the defect.

Related filings:#8463 (finding — a JSDoc that would lure a future maintainer into the rejected option 1), #8468 (needs-user-decisionsys_position may be a third instance), #8470 (bug — the residual above).

Marking ready and enqueueing. Fixes #8323 closes the card on merge.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 13, 2026 14:36
@os-zhuang
os-zhuang added this pull request to the merge queueAug 13, 2026
Merged via the queue into main with commit d71ff32Aug 13, 2026
26 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-8323-tenant-scoped-unique-indexes branch August 13, 2026 15:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/xlteststooling

Projects

None yet

2 participants

@os-zhuang@claude