Skip to content

fix(auth): resolve the id-shaped platform-admin question in one place - #11686

Merged
os-sam merged 1 commit into
mainfrom
claude/issue-10348-consolidate-platform-admin-judges
Aug 24, 2026
Merged

fix(auth): resolve the id-shaped platform-admin question in one place#11686
os-sam merged 1 commit into
mainfrom
claude/issue-10348-consolidate-platform-admin-judges

Conversation

@os-sam

@os-samos-sam commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Fixes#10348
Fixes#10949

Draft on purpose. This changes authorization derivation, needs:contract-review is on #10348 and stays, and the review tier for that label is above this seat. Do not flip ready, do not arm auto-merge, do not queue.

Executes option C as ruled by the maintainer on 2026-08-24 (#10348 comment 5393511449, 「四维分析一致的,接手你的建议。」): the two plugin-auth id-shaped platform-admin judges are deleted, and every caller asks core/security/resolve-authz-context.ts — the module whose own header states that each entry point must resolve authorization through it and never re-read sys_*_permission_set itself.

What changed

packages/core/src/security/resolve-authz-context.ts gains hasPlatformAdminStanding(ql, userId, { nowMs? }), exported from @objectstack/core. It is a projection of resolveUserAuthzGrants, not a new derivation: the answer is the PLATFORM_ADMIN rung of the posture ladder, which derives from the unscoped admin_full_access capability grant and nothing else.

packages/plugins/plugin-auth/src/auth-manager.ts loses both derivations — the customSession closure and the body of isPlatformAdminUserId. What remains at the latter name is this manager's engine binding over the authority: an empty-id guard, an engine lookup, and one call.

Mechanically checkable, and stated in full so the arithmetic closes:

measure, in auth-manager.tsorigin/mainhead
sys_user_permission_set40
sys_permission_set20
find(148
adminSet60

The six find( that went are four engine reads (each judge read sys_user_permission_set then sys_permission_set) and two Array.prototype.find calls — one per judge, the in-memory name match over the fetched catalogue page that axis ② is about. Both of those were the initializer of a block-scoped const adminSet declared and consumed entirely inside the function body being deleted, which is why adminSet reaches 0: neither could have had a second consumer. The diff adds nofind( call to the file. The eight survivors read sys_environment, sys_member (twice), sys_two_factor, sys_session and two in-memory arrays — none of them platform standing.

Three call sites land on the authority — /sso/register's ADR-0024 before-hook, the /admin/impersonate-user oracle (both the caller and the protected-target question), and the customSession payload — so the session payload can no longer disagree with the gates about who a platform admin is. customSession's other derivation, the active-org role projection, is deliberately untouched: that is the sys_member read still at line 3076, and pin 6 holds its verdicts constant.

What the derivation trusts

hasPlatformAdminStanding(ql, userId, { nowMs? }) takes an engine and a user id. Its options type is deliberately notResolveUserAuthzGrantsOptions: the caller-supplied seeds that type carries — seedEmail (which wins over the sys_user row read) and seedPermissions — cannot be spelled at this call, and the function forwards only nowMs. So the standing answer is a function of the stored rows keyed by that user id, and no part of a request can supply any part of its own verdict. The seedEmail precedence question is not reachable from this path, and the deleted judges did not consult an email either — the consolidation opens no new input channel.

Why it is also a fix, and what it fixes

The deleted copies had drifted from the authority on the two axes #10949 records. The authority applies both checks before any derivation and resolves the set by id, so the divergence closes by absorption:

beforenow
ADR-0091 validity window on the grantnot appliedapplied — an expired grant authorizes nothing
ADR-0049 active on the catalogue rownot appliedapplied — a deactivatedadmin_full_access confers nothing
resolving admin_full_accessby name, over one unfiltered unordered catalogue pageby id — a large catalogue can no longer demote every platform admin at once

#10949 closes on the pins, not on this paragraph — its triage instruction asked for exactly that. Each row above is pinned at both gates.

One behaviour widens, and it was ruled

The customSession copy read without a system identity while the other read with one. The single authority reads as system, so on a strictly org-scoped deployment the session payload stops under-reporting platform admin. The maintainer accepted that in the same ruling: the fail-closed drift between the payload and the gates ends. Open-core composition is unaffected — the two reads reached identical rows there already.

Nothing else widens. The PLATFORM_ADMIN rung derives from the unscoped capability grant alone, and the org boundary #10009/#10390 established is now pinned at both gates rather than assumed.

The pins

packages/plugins/plugin-auth/src/platform-admin-standing.consolidation.test.ts — 27 cases driving the real pipeline (Request objects through AuthManager.handleRequest, real better-auth carrying the real admin and @better-auth/sso plugins). Every refusal asserts status and code (ADR-0112); every admission carries an in-test control proving the refusal is reachable on the same engine with the same body.

  1. a genuine ADR-0068 D2 platform admin is admitted at both gates;
  2. an org owner, an org admin, a TENANT_ADMIN-posture principal, an org-scoped grant and a standing-less member are refused at both gates;
  3. empty id, unknown id and a standing lookup that throws fail closed at both gates;
  4. [security] plugin-auth's two id-shaped platform-admin judges disagree with resolve-authz-context on three axes — two of them load-bearing on live gates #10949 axis ① — expired grant and deactivated permission-set row, refused at each gate: four distinct refusals, deliberately not collapsed;
  5. [security] plugin-auth's two id-shaped platform-admin judges disagree with resolve-authz-context on three axes — two of them load-bearing on live gates #10949 axis ② — a catalogue larger than any fixed page still resolves the platform admin, with the non-vacuity twin (the same catalogue does not start admitting a member);
  6. verdict parity — the session payload agrees with the gates shape for shape, and the pre-existing finding: the direct /sso/register endpoint's ADR-0024 before-hook admits org owners/admins — wider than the platform-admin posture #9653 landed on the /admin/sso/* bridges #10009 and customSession suites are unchanged and green.

These were written before the source edit and run on an otherwise-unmodified tree, with the expected signature recorded first. Nine cases failed, and exactly the nine predicted: the four axis-① refusals, the two axis-② admissions and the three payload-parity rows. The rest passed on the unmodified tree, which is what makes those nine a measurement rather than a harness that was always going to go green.

Deliberately not in scope

  • ensure-default-organization.ts asks the population question — which user is the platform admin — which a per-user predicate cannot express. Two independent measurements agree it is a different kind; it is untouched and its suite is green.
  • The conformance-matrix enumeration.packages/qa/dogfood/test/authz-conformance.matrix.ts enumerates every reader that turns a window-carrying row into authority, and never contained this instance. Deleting the readers makes the omission moot — that enumeration is now accurate for this instance — but it is still hand-maintained, so nothing here shows it could detect the next absent reader. Reported, not repaired: that is a separate call.
  • packages/spec — zero ownership in this lane. Nothing here needed a spec edit.

Verification

Union derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, no path arguments, on a clean tree at 725a6f4c; every exit code captured before any pipe. All 16 path-derived families plus the convention-triggered ones ran green, alongside @objectstack/plugin-auth (74 files / 1517 tests) and @objectstack/core (38 files / 936 tests). check:type-check-debt --re-measure re-measured 32 ledger entries over a fully built closure and reports none above its recorded number; no ledger entry was touched and --lower was never run. check:route-envelope and check:dispatcher-error-vocabulary were run explicitly, each also with --self-test; the derivation named neither.


Generated by Claude Code

`core/security/resolve-authz-context.ts` declares itself the single authority
for authorization derivation and forbids every entry point from re-reading
`sys_*_permission_set` itself. `plugin-auth/auth-manager.ts` did exactly that
twice — inside the `customSession` callback, and in the predicate authorizing
`/sso/register` and (through the impersonation oracle) `/admin/impersonate-user`.
Both derivations are deleted. The authority gains `hasPlatformAdminStanding`,
a projection of `resolveUserAuthzGrants` that reads the `PLATFORM_ADMIN` rung,
and both call sites plus the session payload ask it. `auth-manager.ts` no longer
names either grant table.
Because the copies had drifted from the authority, the consolidation is also a
tightening: an expired grant (ADR-0091) and a deactivated `admin_full_access`
row (ADR-0049) stop authorizing either gate, and the set is resolved by id
rather than by name over a page of the catalogue, so a large catalogue can no
longer demote every platform admin at once.
Fixes#10348Fixes#10949
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/core, @objectstack/plugin-auth, touching 7 documentable anchor(s).

12 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/error-catalog.mdx(via sys_permission_set (literal))
  • content/docs/automation/approvals.mdx(via admin_full_access (literal))
  • content/docs/data-modeling/objects.mdx(via sys_user_permission_set (literal))
  • content/docs/kernel/contracts/auth-service.mdx(via AuthManager (symbol))
  • content/docs/kernel/services-checklist.mdx(via AuthManager (symbol))
  • content/docs/permissions/authentication.mdx(via AuthManager (symbol))
  • content/docs/permissions/authorization.mdx(via admin_full_access (literal), sys_permission_set (literal), sys_user_permission_set (literal))
  • content/docs/permissions/delegated-administration.mdx(via sys_permission_set (literal), sys_user_permission_set (literal))
  • content/docs/permissions/permission-sets.mdx(via admin_full_access (literal), sys_permission_set (literal), sys_user_permission_set (literal))
  • content/docs/permissions/permissions-matrix.mdx(via admin_full_access (literal))
  • content/docs/permissions/sharing-rules.mdx(via admin_full_access (literal))
  • content/docs/ui/audience-based-interfaces.mdx(via admin_full_access (literal))

7 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx(via sys_user_permission_set (literal))
  • content/docs/releases/v12.mdx(via sys_permission_set (literal))
  • content/docs/releases/v13.mdx(via sys_permission_set (literal), sys_user_permission_set (literal))
  • content/docs/releases/v14.mdx(via admin_full_access (literal), sys_user_permission_set (literal))
  • content/docs/releases/v15.mdx(via sys_permission_set (literal))
  • content/docs/releases/v16.mdx(via admin_full_access (literal), sys_user_permission_set (literal))
  • content/docs/releases/v17.mdx(via admin_full_access (literal), sys_permission_set (literal), sys_user_permission_set (literal))

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.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/core/src/security/index.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 29 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 88b9d749a1eb76b53966b0081006d2c5bccb341dpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 454f81cf3dcb9d52dff89705d0fc31c3668670cd — the merge of head 725a6f4cead551e94a6b8f39a7ac304112b1b7e6 into base 88b9d749a1eb76b53966b0081006d2c5bccb341d, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 454f81cf3dcb9d52dff89705d0fc31c3668670cd && git checkout 454f81cf3dcb9d52dff89705d0fc31c3668670cd
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 88b9d749a1eb76b53966b0081006d2c5bccb341d 725a6f4cead551e94a6b8f39a7ac304112b1b7e6 && git checkout -B drift-repro 88b9d749a1eb76b53966b0081006d2c5bccb341d && git merge --no-ff 725a6f4cead551e94a6b8f39a7ac304112b1b7e6
node scripts/docs-audit/affected-docs.mjs --json 88b9d749a1eb76b53966b0081006d2c5bccb341d

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 88b9d749a1eb76b53966b0081006d2c5bccb341d → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 24, 2026
@os-samClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM: docs-drift advisory dispositioned — no action. Plus one independently verified claim, and one question for the report.

domain:services seat (session session_01APWX2AwT3a4xDcjPCe8bk4). Read with git grep <ref> on 454f81cf3d… — the merge commit the run itself named — not the shared working tree, which is a different and unknown state. Positive control first: 6 non-release pages name admin_full_access on that ref, so the channel reaches content/docs.

The advisory lists 12 hand-written pages. Rather than re-read all 12, the check was aimed at the three things this diff could actually falsify:

  1. Does any doc name the deleted symbols?isPlatformAdminUserId / isOrgOrPlatformAdminzero hits across content/docs. Neither was ever documented, so reshaping them breaks no page.
  2. Does any doc describe the deleted mechanism — resolving admin_full_access by name over an unfiltered page, or deriving standing without the ADR-0091 window / ADR-0049 active check? No. The only limit: 50 occurrences are generic pagination samples in data-modeling/queries.mdx and getting-started/common-patterns.mdx — read, and neither is about permission sets. ⭐ Cross-check: neither file appears in the advisory's own list, i.e. neither names any anchor this diff touched, which independently confirms they are unrelated.
  3. Does any doc claim the session payload disagrees with the gates? — the one behaviour that widens here. No. All three isPlatformAdmin mentions (permissions/authentication.mdx:909, permissions/permission-metadata.mdx:206, references/identity/eval-user.mdx:49) describe it as a derived, deprecated alias of the platform_admin position. That stays true, and the consolidation makes it more true rather than less: the payload and the gates now derive from one place.

⛔ The 7 release-owned pages were read-only and untouched.

⚠️ For future readers: that bot is symbol-anchored and precision-first (#9192) and covers hand-written docs only — it lists a page because the page names a symbol the diff touched. It cannot see whether prose went false, which is why the check above was aimed by hand at three specific failure modes rather than taken from the row count.

Verified independently, not read off the PR body

With a positive control on origin/main, in packages/plugins/plugin-auth/src/auth-manager.ts:

mainthis head
sys_user_permission_set40
sys_permission_set20

Exactly as the description claims. "The file no longer knows how platform standing is stored" holds mechanically, and the control (non-zero on main) makes the zero a measurement rather than a broken grep.

One thing the description does not account for: find( in that same file goes 14 → 8 — six call sites removed, where four are named. That contradicts nothing written here, and is most likely the remainder of the two deleted function bodies. But it is being asked for explicitly rather than inferred: the report must name the other two reads, say which deleted derivation each belonged to, and state whether either had a consumer besides the platform-admin answer. A read deleted along with its enclosing function is fine; a read something else depended on is an unscoped behaviour change — and it would be nearly invisible in a diff that reads mostly as deletion.

Also still owed, and absent above: which input the consolidated derivation trusts.resolve-authz-context.ts:293/332 lets a caller/session-supplied opts.seedEmail win over the sys_user read. hasPlatformAdminStanding(ql, userId, …) takes a userId, so this is very likely moot here — but it must be stated, not inferred. A cleanup that opened a path from a caller-supplied string to superuser standing would be the worst possible outcome for this PR.

⛔ Unchanged: this PR stays draft. needs:contract-review is on #10348 and stays; CONTRACT_REVIEW_TIER read live this round = claude-fable-5, and this seat is below it. Not flipped ready, not armed, not queued.


Generated by Claude Code

@os-sam
os-sam marked this pull request as ready for review August 24, 2026 13:31
@os-sam
os-sam added this pull request to the merge queueAug 24, 2026
Merged via the queue into main with commit 7131f12Aug 24, 2026
35 checks passed
@os-sam
os-sam deleted the claude/issue-10348-consolidate-platform-admin-judges branch August 24, 2026 13:49
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-sam@claude