feat(spec,plugin-security): publish the caller's resolved permission SETS on the security service (#7616) - #7829
Open
claude[bot] wants to merge 2 commits into
Open
Conversation
…SETS on the security service (#7616) `ISecurityService` could report the caller's effective permission-set NAMES (`resolvePermissionSetNames`) and nothing else — the right primitive for an audience check, the wrong one for a merge. A consumer folding the caller's grants into one answer needs `objects`, `fields`, `systemPermissions` and `tabPermissions`; none of the four is reachable from a name, so `/auth/me/permissions` and `/me/apps` each re-implement the resolution locally instead. One rule, three copies — drifted from the enforcement path three times (#7608, #7555, #6334), each divergence found only after it reached a user. Adds `resolvePermissionSetsForContext(context)` to the published contract: the same resolution the names surface reports the names of, returned whole and in resolution order. Merge semantics stay with the caller, because two consumers legitimately project different subsets of the same sets. The method is OPTIONAL, and that is load-bearing: the contract's availability rule has consumers resolve this service as `Partial<ISecurityService>`, so a caller must keep its own resolution as the fallback until a floor version carrying the method can be assumed. Optional makes that degradation a property of the type — the unguarded call does not compile. `plugin-security` exposes it on the REGISTERED SERVICE LITERAL, not merely as a public class member. The consumers must never take a runtime dependency on this plugin (optional in the stacks those endpoints serve), so the service locator is the only seam that can carry the delegation, and a method the class declares but the literal does not expose is unreachable across it. Pinned by a test that resolves the service off `registerService` and calls through that handle; reverse-verified (all 5 cases fail without the exposure). Also closes one implementation gap so the declaration is true rather than nominal: the `sys_permission_set` loader dropped `tab_permissions`, so every DB-authored set came back without the column `/me/apps` filters its app list with. Inert for enforcement (nothing on the data plane reads `tabPermissions`), no extra query — the row is already fetched in full. No behaviour change today: the method has no caller yet, by design. The call sites are step 2 and land separately, because `/me/apps` deliberately projects a narrower column set than `/auth/me/permissions` and delegating changes which columns load on both surfaces — a user-visible change wanting its own before/ after measurement. Also corrects a stale `resolveFallbackPermissionSets` doc-comment describing the `resolved.length === 0 && fallbackName` step PR #7615 deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rK3z836HF4CjfpbJuEjaz
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 109 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
github-actions
Bot
added
documentation
os-zhuang
marked this pull request as ready for review
August 11, 2026 17:52
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the step 1 half of #7616. Step 2 (switching the call sites in
plugin-hono-server'scurrent-user-endpoints.tsand deleting the local resolution) is deliberately not here — see "What this does not do".Why
ISecurityServicecould report the caller's effective permission-set names (resolvePermissionSetNames) and nothing else. That is the right primitive for an audience check — "does this caller holdsales_manager?" — and the wrong one for a merge. A consumer folding the caller's grants into one answer needs the sets themselves:objects,fields,systemPermissions,tabPermissions. None of the four is reachable from a name.So the two consumers that need a merge re-implement the resolution instead.
/auth/me/permissionsand/me/appseach resolve the caller's permission sets by hand, alongsideSecurityPlugin's own copy on the data plane — one rule, three copies, and it has drifted from the enforcement path three times, each divergence found only after it reached a user:resolved.length === 0fallback cliff/me/appsisDefaultset displacedmember_defaultrather than composing with itsys_user_position/sys_position_permission_setentirelyresolveUserAuthzGrants— the precedent this extends one step furtherWhat changed
packages/spec/src/contracts/security-service.ts— newresolvePermissionSetsForContext?(context): the same resolutionresolvePermissionSetNamesreports the names of, returned whole and in resolution order. Implementations must return the sets their own enforcement path resolved (positions expanded, the D5 baseline applied additively, the D10 agent-principal rule honoured), never a re-derivation. Merge semantics stay with the caller on purpose — two consumers legitimately project different subsets of the same sets, and folding a merge in here would make the method a fourth copy of the rule rather than the one source of its input.It is OPTIONAL, and that is load-bearing. The contract's own availability rule has consumers resolve this service as
Partial<ISecurityService>, so a caller must keep its local resolution as the fallback until a floor version carrying the method can be assumed. Declaring it optional makes that degradation a property of the type — the unguarded call does not compile — rather than a promise in prose. This follows the file's established precedent forgetMetadataReadableFields?(ADR-0106 D7) andcheckAuthoredRowWrite?(#5493), both added optional for exactly this reason.packages/plugins/plugin-security/src/security-plugin.ts— exposed on the registered service literal, not merely as a public class member.plugin-hono-servermust never take a runtime dependency onplugin-security(it is optional in the stacks those endpoints serve — the!evaluatordegraded branches are exactly its absence), so the service locator is the only seam that can carry the delegation, and a method the class declares but the literal does not expose is unreachable across it. The class method staysprivate: the literal is the supported surface.Same file — one implementation gap closed so the declaration is true rather than nominal. The plugin's
sys_permission_setloader hydratedobjects,fieldsandsystemPermissionsbut droppedtab_permissions, so every DB-authored set came back without the column/me/appsfilters its app list with. Shipping a contract that promises the sets whole over a loader that drops a quarter of them is the declared-≠-delivered defect Prime Directive #10 exists to prevent. Nothing on the data plane readstabPermissions(the evaluator never mentions it), so this is inert for enforcement today; the row is already fetched in full, so it is one JSON parse and no extra query.Same file — the comment-only fold-in the card asks for.
resolveFallbackPermissionSets' doc-comment still described "the second step/auth/me/permissionstakes when a caller's own names resolve to nothing (resolved.length === 0 && fallbackName)". PR #7615 deleted that step — the guard was the fallback cliff D5 abolishes. Corrected, plus a second stale cross-reference to "the same two-step" further down the same comment.Tests —
security-service.test.tspins the optionality (the unguarded call must not compile) and the four columns the names surface cannot reach.resolve-permission-sets-for-context.pin.test.ts(new, inplugin-security) pins declared = reachable.The pin: declared = reachable
An unused method still has something worth pinning, and it is the part that actually breaks. Every case in the new pin test resolves the service the way a cross-package consumer does — off the
ctx.registerService('security', …)call, as aPartial<ISecurityService>— and never off the plugin instance:typeof svc.resolvePermissionSetsForContext === 'function');sys_permission_setthrough the DB loader — the path where a column silently goes missing);sets.map(s => s.name)deep-equalsresolvePermissionSetNames), so a future edit cannot give the two surfaces separate resolutions;member_default(the/auth/me/permissionsand/me/appsstill apply the baseline only when the caller resolves to ZERO sets — the ADR-0090 D5 fallback cliff, one plane over from where it was abolished #7608 shape);Reverse-verified, both halves:
resolvePermissionSetsForContextentryexpected 'undefined' to be 'function', and every call returns nothingtabPermissionshydrationexpected undefined to deeply equal { app_crm: 'visible' }Both restored and re-confirmed green afterwards.
What this does not do — and why
⛔ The call sites are untouched.
current-user-endpoints.tsis not in this diff, and the local resolution is not deleted./me/appsdeliberately projects a narrower column set (name+systemPermissions+tabPermissions) than/auth/me/permissionsdoes. Delegating changes which columns load on both surfaces — a user-visible behaviour change that wants its own before/after measurement, not a refactor riding on a contract addition. Landing both steps together is how this becomes an unreviewable diff.The honest tension, stated rather than papered over
This ships a contract method with no caller, and it is a net addition rather than a removal. Because the contract mandates feature-detection, the local copy in
current-user-endpoints.tsmust stay as the fallback for aSecurityPlugintoo old to carry the method — so for now the repo has the copy plus the delegation path plus a branch between them. De-duplication that adds a third path is not de-duplication yet.It is still the right first move: step 2 is unreachable until a floor version carrying the method can be assumed, and this is what makes that floor exist. Nothing about this PR changes behaviour today — it is not a fix, and the report on #7616 says so plainly rather than claiming one.
Gates (measured)
pnpm lint(repo-wide, required)pnpm --filter @objectstack/spec typechecktsc+ scripts + test layer)pnpm --filter @objectstack/plugin-security typecheckpnpm --filter @objectstack/spec check:generatedcheck:exported-anyanycheck:dual-source-exportscheck:test-typecheck(spec, exact ratchet)pnpm check:type-check-coveragecheck:service-providers,check:init-service-contract,check:authz-resolver,check:role-word,check:adr-anchors,check:tenant-chokepoint,check:nul-bytes,check:empty-changesetpnpm --filter @objectstack/plugin-security testpackages/speccontract testNo gate objected to the unused method.
check:livenessgoverns authorable metadata-type properties against the registry, notcontracts/interfaces, and it is green. No ratchet ledger was raised — the one exact ratchet in the blast radius (test-typecheck-debt.json) came out byte-identical.Two findings, reported not fixed
packages/spec/src/contracts/security-service.test.ts's "exhaustive" method list is not exhaustive, onmainas well as here.hasWriteBypass,resolveWriteScopeanddescribeDelegableScopeare required onISecurityServiceand appear in neither the assertion list normakeService()'s literal — which is why that file carries exactly 1 held error intest-typecheck-debt.json(TS2322at line 21: the stub is not assignable toISecurityService). The compile-time half of that test is currently being asserted past a suppressed error. Adding an optional method changes neither the runtime assertion nor the error count, so nothing here goes red — but "the suite goes red if you don't update the list" is not true today, and the reason it isn't is a pre-existing defect. Left alone deliberately: fixing it takes the ledger 1 → 0 and belongs to whoever owns those three methods, not to a contract addition.mainis red on one spec test, unrelated to this diff:src/data/api-methods-batch-conformance.test.ts—sys_api_key: [get, list, update] grants single-record writes but not 'bulk'(packages/platform-objects/src/identity/sys-api-key.object.ts, a file this PR does not touch and which is byte-identical tomain).Generated by Claude Code